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(viem): Adding standby tx after sending #4443

Merged
merged 12 commits into from
Nov 15, 2023
Merged

fix(viem): Adding standby tx after sending #4443

merged 12 commits into from
Nov 15, 2023

Conversation

dievazqu
Copy link
Contributor

@dievazqu dievazqu commented Nov 7, 2023

Description

Adding the standby transaction only after the transaction has been sent to the blockchain in the viem flow.

Test plan

Run locally in a emulator and unit tests

Related issues

Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2023-11-14.at.05.49.05.mp4

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #4443 (a5a6fed) into main (cab4bcd) will decrease coverage by 0.02%.
The diff coverage is 73.68%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4443      +/-   ##
==========================================
- Coverage   85.20%   85.19%   -0.02%     
==========================================
  Files         712      712              
  Lines       26587    26596       +9     
  Branches     3624     3627       +3     
==========================================
+ Hits        22653    22658       +5     
- Misses       3871     3875       +4     
  Partials       63       63              
Files Coverage Δ
src/viem/saga.ts 94.89% <100.00%> (+0.95%) ⬆️
src/transactions/reducer.ts 56.94% <0.00%> (-3.35%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cab4bcd...a5a6fed. Read the comment docs.

Copy link
Collaborator

@kathaypacific kathaypacific left a comment

Choose a reason for hiding this comment

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

this seems good to me, would be good to get @jeanregisser 's eyes on this too. last time i missed some details in #4395 🙈

Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Thanks!

I'm slightly confused by the example video because it shows the swap flow, but right now it doesn't use src/viem/saga.ts, which was changed in this PR.

Did I miss something?


const addPendingStandbyTransaction = function* (hash: string) {
Copy link
Member

@jeanregisser jeanregisser Nov 9, 2023

Choose a reason for hiding this comment

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

In theory the addStandbyTransaction action could be called mutliple times, because it's wrapped in

for (let i = 1; i <= TX_NUM_TRIES; i++) {

However, right now this TX_NUM_TRIES is set to 1.

I think just to be safe, we should update the reducer to replace any standby TX which has the same hash or context id.

case Actions.ADD_STANDBY_TRANSACTION:
return {
...state,
standbyTransactions: [
{
...action.transaction,
timestamp: Date.now(),
status: TransactionStatus.Pending,
},
...(state.standbyTransactions || []),
],
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

@dievazqu
Copy link
Contributor Author

dievazqu commented Nov 13, 2023

Thanks!

I'm slightly confused by the example video because it shows the swap flow, but right now it doesn't use src/viem/saga.ts, which was changed in this PR.

Did I miss something?

@jeanregisser Sorry, you are right, I have reused the video that I created when I was testing the swap flow. But this PR doesn't change that flow. I'll update the video to show the send flow (similar to what I have shown in the demo).

@dievazqu dievazqu enabled auto-merge (squash) November 14, 2023 13:26
@dievazqu dievazqu disabled auto-merge November 14, 2023 13:33
@dievazqu
Copy link
Contributor Author

@jeanregisser I added the logic in the reducer as you suggested, I'm leaving the PR open in case you want to check that logic. Otherwise, I'm gonna merge this at EOD.

@dievazqu dievazqu enabled auto-merge (squash) November 15, 2023 07:23
@dievazqu dievazqu disabled auto-merge November 15, 2023 07:23
@dievazqu dievazqu enabled auto-merge (squash) November 15, 2023 07:24
@dievazqu dievazqu merged commit 2242d94 into main Nov 15, 2023
16 checks passed
@dievazqu dievazqu deleted the failed-standby-txs branch November 15, 2023 08:12
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.

3 participants