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: error in navigating between transaction when one of the transaction is approve all #27985

Merged
merged 4 commits into from
Oct 21, 2024

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Oct 21, 2024

Description

Navigation in transaction header was broken when one of the transaction is of type Approve All

Related issues

Fixes: #27913

Manual testing steps

  1. Go to test dapp
  2. Submit any re-designed transaction followed by an approve all transaction
  3. Try navigating between transaction using header buttons

Screenshots/Recordings

TODO

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@jpuri jpuri added the team-confirmations Push issues to confirmations team label Oct 21, 2024
@jpuri jpuri requested a review from a team as a code owner October 21, 2024 11:51
Copy link
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.

@@ -174,11 +174,10 @@ const ConfirmTitle: React.FC = memo(() => {

let isRevokeSetApprovalForAll = false;
let revokePending = false;
const decodedResponse = useDecodedTransactionData(TransactionType.tokenMethodSetApprovalForAll);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason for the big for that this hook was being called conditionally

DecodedTransactionDataResponse | undefined
> {
export function useDecodedTransactionData(
transactionType?: string,
Copy link
Member

Choose a reason for hiding this comment

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

To make this more explicit and readable, could we use an object argument call this something like transactionTypeFilter?

We could also omit the filter completely and always decode but I appreciate this is a nice optimisation.

If we end up with many of usages of this hook, we may want to implement some kind of caching in the underlying background utils, or even move this to the TransactionController.

@jpuri jpuri added this pull request to the merge queue Oct 21, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [14a5895]
Page Load Metrics (2085 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26028251909568273
domContentLoaded18012680204718086
load18222703208517785
domInteractive2495492010
backgroundConnect972372110
firstReactRender63133100178
getState6100322914
initialActions01000
loadScripts13242013152114168
setupStore1378352311
uiStartup205930352322208100
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 82 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Merged via the queue into develop with commit 81f4678 Oct 21, 2024
76 checks passed
@jpuri jpuri deleted the nav_fix branch October 21, 2024 13:10
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2024
@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Oct 21, 2024
@gauthierpetetin gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.8.0 Issue or pull request that will be included in release 12.8.0 labels Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-confirmations Push issues to confirmations team
Projects
None yet
5 participants