-
Notifications
You must be signed in to change notification settings - Fork 80
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
refactor(send): execute prepared transaction in saga #5001
Conversation
src/viem/saga.ts
Outdated
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 was using the wrapSendTransactionWithRetry method, which attempts to retry after a timeout of 90s, but the retry count was set to only 1, all of this is removed in the send saga, which now matches with what the swap saga is doing currently.
Also, this was firing events at each stage of the transaction, but the send saga already covers most of it, we only lose out on some intermediate events,transaction_hash_received
and transaction_receipt_received
, which seem more like debugging events.
I don't think either of these are required anymore, but can add these back if there's a strong reason
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.
Is there harm in including these in the updated saga? Seems like there are clear places where they would fit?
If these are truly never used by anyone though, I think it's fine to get rid of them. Are there any other actual uses of these events in code though? If we remove them here, can we clean them up from the app entirely?
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.
It would be weird just to include those tx events and don't fire other tx events. These aren't fired for swaps, so don't think we need to fire them for sends.
These are still used in the old contract-kit sends, we can clean them up once we remove it.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5001 +/- ##
==========================================
- Coverage 85.54% 85.49% -0.05%
==========================================
Files 724 723 -1
Lines 29612 29462 -150
Branches 5127 5099 -28
==========================================
- Hits 25331 25188 -143
+ Misses 4046 4039 -7
Partials 235 235
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
025f46c
to
f45d812
Compare
1 build increased size
Celo (test) 1.79.0 (144)
|
Item | Install Size Change |
---|---|
📝 splashBackground@3x.jpg | ⬆️ 600.2 kB |
📝 background@3x.jpg | ⬆️ 368.6 kB |
📝 boost-rewards@3x.png | ⬆️ 188.4 kB |
📝 background@2x.jpg | ⬆️ 176.1 kB |
📝 boost-rewards@2x.png | ⬆️ 90.1 kB |
🛸 Powered by Emerge Tools
} | ||
} else { | ||
throw new Error('No address found on recipient') | ||
if (tokenInfo?.symbol === 'CELO') { |
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.
nit: Maybe worth having a test for this event?
src/viem/saga.ts
Outdated
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.
Is there harm in including these in the updated saga? Seems like there are clear places where they would fit?
If these are truly never used by anyone though, I think it's fine to get rid of them. Are there any other actual uses of these events in code though? If we remove them here, can we clean them up from the app entirely?
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.
Great!!
Looking forward to when we can combine the logic for send/swap/wc too, but that will be more work :D
src/send/saga.ts
Outdated
) | ||
|
||
if (receipt.status === 'reverted') { | ||
throw new Error('transaction reverted') |
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.
nit: would be nice to include the txHash in the error message for debugging purposes.
src/send/saga.ts
Outdated
yield* put(showErrorOrFallback(err, ErrorMessages.SEND_PAYMENT_FAILED)) | ||
yield* put(sendPaymentFailure()) |
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.
Looks like we wouldn't want the error message if the pin was cancelled.
And maybe not the sendPaymentFailure()
? Unless it's used for switching the button state back to enabled.
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.
The showErrorOrFallback will show a pin required error message, and sendPaymentFailure
just resets the isSending
state so should be fine to include.
I was confused by this as well originally 😄. left a couple of comments
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.
ok, I guess what I'm suggesting is we change the UX to not display an error message if the pin was cancelled, and not proceed. It's slightly annoying to see the error in that case, since the user decided to cancel.
We did that for swap.
Lines 378 to 383 in fb39d99
} catch (err) { | |
if (err === CANCELLED_PIN_INPUT) { | |
Logger.info(TAG, 'Swap cancelled by user') | |
yield* put(swapCancel(swapId)) | |
return | |
} |
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.
makes sense, I think there are a few differences with how send and swap UX works, like for swap we navigate immediately after submitting the tx to the chain but we block on the confirmation screen for send. Would be good to address all that in a separate PR with all the UX changes.
} | ||
Logger.error(`${TAG}/sendPaymentSaga`, 'Send payment failed', error) | ||
ValoraAnalytics.track(SendEvents.send_tx_error, { error: error.message }) | ||
SentryTransactionHub.finishTransaction(SentryTransaction.send_payment) |
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.
Should the sentry transaction be in a finally
block?
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 was my original plan, but if the pin is cancelled, we don't start the transaction, so I included it here. But looking at finishTransaction, it looks like it does nothing if a tx with given name isn't found, so should be safe and clean to do it in finally.
### Description Execute prepared tx directly in the send saga instead of using the viem/saga's send payment method, which was simulating a contract call and then executing it. ### Test plan Manual, CI ### Related issues - Fixes ACT-1049 ### Backwards compatibility Yes ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [X] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)
Description
Execute prepared tx directly in the send saga instead of using the viem/saga's send payment method, which was simulating a contract call and then executing it.
Test plan
Manual, CI
Related issues
Backwards compatibility
Yes
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: