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 date displayed with no Tx #434

Merged
merged 23 commits into from
Nov 20, 2023
Merged

Fix date displayed with no Tx #434

merged 23 commits into from
Nov 20, 2023

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Nov 9, 2023

closes #430

This was more complex than expected. The problem was related to a race condition between

  • getting the pending TX from the chain (querying the multisig storage)
  • getting the call data info from the chain (involving querying older blocks and get the calldata from the asMulti calls)

The second call can take time, and when changing accounts, a reset wouldn't be performed. Now since Promises can't be stopped, this resulted in the second promise resolving with outdated data. I solved it in making the 2 calls sequential and less dynamic. It's not really an issue since they should strictly follow each other. I've been consolidating all this logic which used to be separated, into the usePendinTx hook. It's a lot, but it should be more comprehensible already.

To see the bug, you'd have to:

  • have many pending TXs (I had 10) and at least 1 from the multisig (as opposed to from the proxy). You can try by watching 5EMm18Z8WWWT2wit1RxpoZv39goPdYmSksnDTePYAzrUhdJv on Rococo.
  • then you should select the account with the many tx in the UI, wait for them to show, and switch to an account that doesn't have any pending tx. --> you'd see the date with no tx.

Submission checklist:

Compatibility

  • Functionality of change validated with a connected account with multisig
  • Applicable elements hidden / disabled for watched multisigs / pure
  • Looks good for solo multisig
  • Looks good for multisig with proxy

@Tbaut Tbaut requested review from asnaith and Lykhoyda November 9, 2023 13:25
Base automatically changed from tbaut-link to main November 9, 2023 14:59
Copy link
Member

@asnaith asnaith left a comment

Choose a reason for hiding this comment

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

Nice! The fix works successfully, I can no longer recreate the issue on this branch

Copy link
Collaborator

@Lykhoyda Lykhoyda left a comment

Choose a reason for hiding this comment

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

@Tbaut looks good! I pushed the PR where I replaced the old AggregatedData with the new interface CallDataInfoFromChain. As I had errors during the build. Please confirm if it's correct

@Tbaut
Copy link
Collaborator Author

Tbaut commented Nov 15, 2023

Thanks for the review. I'll merge this PR next week as it's touching quite some critical logic, and I'm not very available in the next days if something goes wrong.

@Tbaut Tbaut merged commit 643cfe3 into main Nov 20, 2023
7 checks passed
@Tbaut Tbaut deleted the tbaut-fix-wrong-tx branch November 20, 2023 10:44
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.

No transaction but a date is displayed
3 participants