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 getFullTxData so that customTxParamsData is not lost #16805

Merged
merged 2 commits into from
Dec 5, 2022

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Dec 5, 2022

Fixes #16798 (comment)

This bug was introduced here 4245f24#diff-45ae84764c1d49a07813a91adc3351d6ed96bf9ee8fa2ae4c9a116d1e0cba06cR821-R830

Explanation

The bug is that the user inputted token allowance is not the token allowance that ends up being submitted on chain. More precisely, if we show the user the token allowance screen and they customize it, the new txParams.data is not used.

The cause is the above linked codechange whereby an additional parameter was accidentally added to the result function of the selector creator, thereby rendering customTxParamsData undefined. With that parameter removed, customTxParamsData will now be defined as it was before, and will reflect the modified data in the case of a user editing a token allowance amount.

@danjm danjm requested a review from a team as a code owner December 5, 2022 16:50
@danjm danjm requested a review from darkwing December 5, 2022 16:50
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

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.

@seaona seaona added this to the v10.23.0 milestone Dec 5, 2022
@metamaskbot
Copy link
Collaborator

Builds ready [a955e10]
Page Load Metrics (2196 ± 187 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint92104817620297
domContentLoaded152832072180387186
load152832332196390187
domInteractive152832072180387186
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 bytes
  • ui: 0 bytes
  • common: -2 bytes

highlights:

storybook

@@ -851,7 +851,7 @@ export const getFullTxData = createDeepEqualSelector(
return getTransaction(state, transactionId);
},
(_state, _transactionId, _status, customTxParamsData) => customTxParamsData,
(txData, transaction, _status, customTxParamsData) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks right to me when I step through the code while running the example.

I think I was (and maybe still am a bit confused by input vs. output selectors that are passed to the createSelector function as described in the reduxToolkit docs.

@brad-decker are you able to elucidate this pattern a bit better for me?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the last arg passed always the output selector? And then you pass as many input selector before that as there are args? That doesn't seem to match what we have here... I think I'm overcomplicating this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the last arg passed is the output selector, and each of its params is the return value of the previous selectors

Copy link
Contributor

Choose a reason for hiding this comment

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

One second while I look up what 'elucidate' means

Copy link
Contributor

Choose a reason for hiding this comment

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

There are 3 selectors being used in this 'createDeepEqualSelector', 'getTxData', and two inline functions. The fourth argument gets txData from 'getTxData', transaction from the first inline function, and customTxParamsData from the second inline function

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh I see now! Ok feels confusing because the status arg is used in one of the input selectors but not the output selector.

@danjm danjm merged commit 2434c43 into develop Dec 5, 2022
@danjm danjm deleted the fix-token-allowances branch December 5, 2022 19:34
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants