chore(viem-fees): get fee estimates for case where token supports comments #4439
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4439 +/- ##
==========================================
+ Coverage 85.06% 85.08% +0.02%
==========================================
Files 710 710
Lines 26349 26361 +12
Branches 3576 3576
==========================================
+ Hits 22413 22429 +16
+ Misses 3873 3869 -4
Partials 63 63
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| feeCurrencies, | ||
| } | ||
| if (tokenSupportsComments(token)) { | ||
| return prepareTransferWithCommentTransaction(transactionParams) |
There was a problem hiding this comment.
this won't include comments because we only request it on the next screen right? Can fees change based on the length of the comment?
There was a problem hiding this comment.
yeah they can... I'd like to follow up on that in a separate PR since I think this one has reasonable behavior for now
There was a problem hiding this comment.
it's a good point though
There was a problem hiding this comment.
one thing we could do is set a maximum length on comments on the next screen, and on this screen use a dummy comment that is of the max length, which is the most pessimistic scenario for gas fee estimation
There was a problem hiding this comment.
it's a little tricky because we then encrypt the comment, so we can't just use the same constant in both places
There was a problem hiding this comment.
Yes setting a MAX_COMMENT_LENGTH is what the contractkit code did to estimate it:
Line 47 in bf19a7f
I haven't followed the latest send UI changes, but should we maybe tweak the UI flow so this isn't too much of a problem with prepared transactions?
Description
Get fee estimates for c-stables transferWithComment method in select amount screen.
Note that since DEK registration does not actually take place before the send transaction, even if the DEK is not registered on-chain yet, DEK registration fees are not included. More context on DEK's here
Test plan
jest tests
tested manually, looks like this:
Related issues
cc https://linear.app/valora/issue/ACT-955/lazy-viem-fees-for-send-celo-only
Backwards compatibility
na