-
Notifications
You must be signed in to change notification settings - Fork 637
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
Replace App raps with BX #5608
Replace App raps with BX #5608
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks great,
do think we need to finish up the custom gas logic if we use new gas store in these existing flows before we merge this (or remove and use legacy for now)
resolve(); | ||
} else { | ||
// Wait for 1 second and try again | ||
await delay(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems slow to me 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can bump it up, just what the BX had
Replaced new gas store with old redux gas. Will rip old redux stuff out in a future PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested send, wc, accept offer, mint, swap
|
||
const waitForNodeAck = async (hash: string, provider: Signer['provider']): Promise<void> => { | ||
return new Promise(async resolve => { | ||
const tx = await provider?.getTransaction(hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not provider.waitForTransaction
?
https://docs.ethers.org/v5/api/providers/provider/#Provider-waitForTransaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use that and wait for 1 confirmation, however for the sake of keeping code parity between app <> bx I didn't change any of the raps (functionally).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use QA but lgtm
tested all raps entry points and they worked as expected |
Fixes APP-1311
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-08.at.17.25.16.mp4