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.

Sign up for GitHub

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

Jump to bottom

Refactor UpdateEIP1559Transaction Component #4804

Merged
merged 11 commits into from
Oct 4, 2022
Merged

Refactor UpdateEIP1559Transaction Component #4804

merged 11 commits into from
Oct 4, 2022

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Aug 4, 2022 •
edited
Loading

Description
There is very similar code spread across 5 components. The aim of this PR is to reduce the components to use as much common shared code as possible. This PR addresses the UpdateEIP1559Transaction screen. The changes in this PR shouldn't modify existing behaviour when QA'd. Check

Issue

Progresses
https://github.com/MetaMask/mobile-planning/issues/272

Screenshots/Recordings

updateTxn.mov

Issue

Progresses https://github.com/metamask/mobile-planning/issues/272

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 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.

@blackdevelopa blackdevelopa marked this pull request as ready for review August 5, 2022 17:15
@blackdevelopa blackdevelopa requested a review from a team as a code owner August 5, 2022 17:15
@blackdevelopa blackdevelopa self-assigned this Aug 8, 2022
sethkfman
sethkfman reviewed Aug 9, 2022
View reviewed changes
app/components/UI/EditGasFee1559Update/index.tsx Show resolved Hide resolved
app/components/UI/UpdateEIP1559Tx/index.tsx Outdated Show resolved Hide resolved
app/components/UI/UpdateEIP1559Tx/index.tsx Outdated Show resolved Hide resolved
app/core/gasPolling.ts Outdated Show resolved Hide resolved
@blackdevelopa blackdevelopa changed the title Update Transaction Component Refactor EIP1559Transaction Component Aug 9, 2022
sethkfman
sethkfman reviewed Aug 9, 2022
View reviewed changes
app/core/gasPolling.ts Outdated Show resolved Hide resolved
sethkfman
sethkfman reviewed Aug 9, 2022
View reviewed changes
app/core/gasPolling.ts Outdated Show resolved Hide resolved
@sethkfman sethkfman added release-5.7.0 Issue or pull request that will be included in release 5.7.0 needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Aug 9, 2022
@sethkfman sethkfman self-requested a review August 10, 2022 18:37
sethkfman
sethkfman approved these changes Aug 10, 2022
View reviewed changes
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@sethkfman sethkfman added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Aug 10, 2022
@blackdevelopa blackdevelopa changed the title Refactor EIP1559Transaction Component Refactor UpdateEIP1559Transaction Component Aug 10, 2022
@cortisiko cortisiko added the team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead label Aug 12, 2022
@chrisleewilcox chrisleewilcox added QA in Progress QA has started on the feature. QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA in Progress QA has started on the feature. labels Aug 15, 2022
@chrisleewilcox
Copy link
Contributor

chrisleewilcox commented Aug 19, 2022 •
edited
Loading

Works for test networks...

  • Rinkeby
  • Groeli

Not able to see transactions for

  • Binance
  • Polygon

https://recordit.co/AOcQTSurhz

UPDATE: Transactions are now showing as of 8/19.

@blackdevelopa
Copy link
Contributor Author

Hey Chris, can we have a sync on this?

@chrisleewilcox
Copy link
Contributor

"Submitted" transactions are not updating to "Completed" when blockchain shows success. When transaction is in this stated the "Speed Up" and "Cancel" will always show popup for retry but retry fails because transactions is already completed.
https://recordit.co/PrmH6nkGiB

@blackdevelopa
Copy link
Contributor Author

blackdevelopa commented Aug 22, 2022 •
edited
Loading

I am working with the stability team (Tomas) on this. It seems to happen on production (bnb network) as well. Eth network is fine and can be tested.

@chrisleewilcox chrisleewilcox mentioned this pull request Aug 22, 2022
Closed
@chrisleewilcox chrisleewilcox added QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Aug 22, 2022
@chrisleewilcox chrisleewilcox added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 15, 2022
@sethkfman sethkfman removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 20, 2022
@chrisleewilcox chrisleewilcox added QA in Progress QA has started on the feature. Mobile QA board and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Sep 21, 2022
@sethkfman sethkfman added release-5.9.0 Issue or pull request that will be included in release 5.9.0 and removed release-5.8.0 Issue or pull request that will be included in release 5.8.0 team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Sep 22, 2022
chrisleewilcox
chrisleewilcox approved these changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

Approving to see if this run the audit checks again.

@chrisleewilcox chrisleewilcox merged commit 3c353f6 into main Oct 4, 2022
@chrisleewilcox chrisleewilcox deleted the refactor-updateTxns branch October 4, 2022 14:28
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2022
@chrisleewilcox chrisleewilcox added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Oct 4, 2022
@sethkfman sethkfman added release-5.10.0 Issue or pull request that will be included in release 5.10.0 and removed release-5.9.0 Issue or pull request that will be included in release 5.9.0 labels Oct 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sethkfman sethkfman sethkfman approved these changes

@chrisleewilcox chrisleewilcox chrisleewilcox approved these changes

Assignees

@blackdevelopa blackdevelopa

Labels
area-transactions QA Passed A successful QA run through has been done release-5.10.0 Issue or pull request that will be included in release 5.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@blackdevelopa @chrisleewilcox @sethkfman @cortisiko