Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix construct tx #3010

Merged
merged 9 commits into from
Nov 16, 2021
Merged

Fix construct tx #3010

merged 9 commits into from
Nov 16, 2021

Conversation

paweljakubas
Copy link
Contributor

adp-1256
adp-1202 (possibly)

  • I have reused selectAssets in constructTransaction. It is more performant now, also one selection is calculated rather than three which is good as running selection is not deterministic

Comments

Issue Number

@paweljakubas paweljakubas self-assigned this Nov 9, 2021
@paweljakubas
Copy link
Contributor Author

@piotr-iohk please verify if with this fix the problems (both adp-1256 and adp-1202) vanish. Thanks!

Just (ApiPaymentAddresses content) ->
pure $ F.toList (addressAmountToTxOut <$> content)
Just (ApiPaymentAll _) -> do
liftHandler $ throwE $ ErrConstructTxNotImplemented "ADP-909"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ADP-909 is already closed... is it a message that's potentially displayed to the user? If so it needs to be reworded 😅

piotr-iohk pushed a commit that referenced this pull request Nov 9, 2021
piotr-iohk pushed a commit that referenced this pull request Nov 9, 2021
path' == path'' &&
amt' == amt'' &&
assets' == assets''
testEquality _ _ = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could replace this testEquality function with == if we merge the ApiCoinSelectionInput and ApiWalletInput types. See #3012

Copy link
Contributor Author

@paweljakubas paweljakubas Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, we can simplify considerably this checking for equality - did it in last two commits

@paweljakubas paweljakubas force-pushed the paweljakubas/adp-1256/fix-construct-tx branch from 5222e33 to 1b7bb29 Compare November 10, 2021 10:48
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-1256/fix-construct-tx branch from c82f81c to 5d29520 Compare November 11, 2021 17:29
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @paweljakubas

This looks fine to me. It's much nicer now that we have only one API type for an input!

I have only proposed some small fixes to indentation. (Just making everything indent by 4 chars.)

Comment on lines 2127 to 2130
, utxoAvailableForInputs =
UTxOSelection.fromIndex utxoAvailable
, utxoAvailableForCollateral =
UTxOIndex.toUTxO utxoAvailable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
, utxoAvailableForInputs =
UTxOSelection.fromIndex utxoAvailable
, utxoAvailableForCollateral =
UTxOIndex.toUTxO utxoAvailable
, utxoAvailableForInputs =
UTxOSelection.fromIndex utxoAvailable
, utxoAvailableForCollateral =
UTxOIndex.toUTxO utxoAvailable

Comment on lines +2143 to +2150
sel <- liftHandler $
W.assignChangeAddressesWithoutDbUpdate wrk wid genChange utx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sel <- liftHandler $
W.assignChangeAddressesWithoutDbUpdate wrk wid genChange utx
sel <- liftHandler $
W.assignChangeAddressesWithoutDbUpdate wrk wid genChange utx

(Link.decodeTransaction @'Shelley wa) Default decodePayload
let decodedInputs = getFromResponse #inputs rDecodedTxSource

WalletInput <$> expectedInputs `shouldBe` decodedInputs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

Comment on lines 2113 to 2114
( W.assignChangeAddresses genChange sel s
& uncurry (W.selectionToUnsignedTx (txWithdrawal txCtx))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
( W.assignChangeAddresses genChange sel s
& uncurry (W.selectionToUnsignedTx (txWithdrawal txCtx))
( W.assignChangeAddresses genChange sel s
& uncurry (W.selectionToUnsignedTx (txWithdrawal txCtx))

Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I have tested locally and also enabled tests for ADP-1202. Tested against this branch with those tests enabled and it passes now! (here and here)

@paweljakubas paweljakubas force-pushed the paweljakubas/adp-1256/fix-construct-tx branch from 18d2460 to 53388ff Compare November 15, 2021 11:56
@paweljakubas
Copy link
Contributor Author

bors r+

1 similar comment
@paweljakubas
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 16, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 0f3168d into master Nov 16, 2021
@iohk-bors iohk-bors bot deleted the paweljakubas/adp-1256/fix-construct-tx branch November 16, 2021 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants