Ensure smart contract interactions are properly represented on the confirm screen #15446
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
brad-decker
reviewed
Aug 3, 2022
Comment on lines
+50
to
+53
| const RECIPIENT_TYPES = { | ||
| SMART_CONTRACT: 'SMART_CONTRACT', | ||
| NON_CONTRACT: 'NON_CONTRACT', | ||
| }; |
Collaborator
brad-decker
previously approved these changes
Aug 3, 2022
adonesky1
reviewed
Aug 3, 2022
ui/ducks/send/send.js
Outdated
Comment on lines
1889
to
1892
| if (smartContractAddress) { | ||
| dispatch(actions.updateRecipientType(RECIPIENT_TYPES.SMART_CONTRACT)); | ||
| } | ||
| if (smartContractAddress) { |
Contributor
There was a problem hiding this comment.
is there a reason you made a new if (smartContractAddress) conditional? The same if statement is right below on line 1892?
Contributor
Author
There was a problem hiding this comment.
uh, no... I originally had this outside the if block two lines above, and then copy pasted it to here, will refactor
Contributor
Author
There was a problem hiding this comment.
changed in latest commit
adonesky1
reviewed
Aug 3, 2022
ui/ducks/send/send.js
Outdated
| useTokenDetection, | ||
| tokenAddressList, | ||
| isProbablyAnAssetContract, | ||
| smartContractAddress, |
Contributor
There was a problem hiding this comment.
is this being used? Sorry If I'm missing it? validateRecipientUserInput doesn't appear to make use of it?
adonesky1
reviewed
Aug 3, 2022
ui/ducks/send/send.js
Outdated
|
|
||
| const inputIsValidHexAddress = isValidHexAddress(userInput); | ||
| let isProbablyAnAssetContract = false; | ||
| let smartContractAddress; |
Contributor
There was a problem hiding this comment.
since this is not longer passed to the validation method this can go back to being a const inside the if (inputIsValidHexAddress) { conditional
Contributor
Author
There was a problem hiding this comment.
linting should tell me this stuff
adonesky1
previously approved these changes
Aug 3, 2022
Contributor
adonesky1
left a comment
adonesky1
left a comment
There was a problem hiding this comment.
Not a blocker since we're trying to get this out the door but lets note that we should make test cases for this?
Contributor
Author
Absolutely. I am preparing tests for PRs I have merged over the past couple of day, and will submit another PR for those |
brad-decker
previously approved these changes
Aug 3, 2022
brad-decker
approved these changes
Aug 3, 2022
adonesky1
approved these changes
Aug 3, 2022
Collaborator
Builds ready [a3e5d45]
Page Load Metrics (1711 ± 57 ms)
highlights:storybook
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #15413
The problem was that we were representing a contract interaction as a simple send when first arriving at the confirm screen. The solution is to ensure that if we are sending to a contract address, we apply the contract interaction type to the transaction when creating it from the send screen.
Some other changes were needed to get this fully working:
TRANSACTION_TYPES.CONTRACT_INTERACTIONas a valid transaction type when adding a new unapproved transactionisSmartContractAddressfunction which was previously broken because it was attempting to get the wrong property from the result ofreadAddressAsContract()Before:
after-aug3-1250.mp4
After:
after-aug3-1246.mp4