[SDK] get ERC20Value for claimToBatch #5157
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Your org has enabled the Graphite merge queue for merging into mainAdd the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
kien-ngo
requested review from
joaquim-verges,
gregfromstl and
jnsdls
as code owners
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| overrides: { | ||
| erc20Value: () => getERC20Value(options), | ||
| ...options.content, | ||
| }, |
There was a problem hiding this comment.
There appears to be a bug where options.content is being spread into the overrides object. This is incorrect as content contains claim parameters, not transaction overrides. The spread should be ...options.overrides instead to properly pass through any transaction override parameters.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Holy cow good bot!!!!
kien-ngo
force-pushed
the
kien/get-erc20Value-for-batch
branch
from
1c3e58b
to
550be2c
Compare
kien-ngo
force-pushed
the
kien/get-erc20Value-for-batch
branch
from
550be2c
to
e849966
Compare
kien-ngo
force-pushed
the
kien/get-erc20Value-for-batch
branch
2 times, most recently
from
6090c75
to
bc3f390
Compare
size-limit report 📦
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5157 +/- ##
==========================================
+ Coverage 45.42% 45.43% +0.01%
==========================================
Files 1065 1065
Lines 55145 55181 +36
Branches 3970 3977 +7
==========================================
+ Hits 25049 25072 +23
- Misses 29404 29417 +13
Partials 692 692
*This pull request uses carry forward flags. Click here to find out more.
|
kien-ngo
force-pushed
the
kien/get-erc20Value-for-batch
branch
from
bc3f390
to
7e93768
Compare
kien-ngo
added
the
DO NOT MERGE
Graphite App
| amountWei: bigint; | ||
| tokenAddress: Address; | ||
| } | ||
| | Promise< |
There was a problem hiding this comment.
make sure we're handling this being a promise where its used
There was a problem hiding this comment.
so far I believe only getApprovalForTransaction is using it (which is the reason why you added erc20Value in the first place?) - I'll double check anyway
Problem solved
Short description of the bug fixed or feature added
PR-Codex overview
This PR focuses on enhancing the
erc20Valuetype inprepare-transaction.tsand adding a newgetERC20Valuefunction inclaimToBatch.tsto handle promise resolutions for ERC20 values in batch claims.Detailed summary
erc20Valuetype to allow a promise or undefined inprepare-transaction.ts.claimToBatchfunction to includeerc20Valuein the overrides.getERC20Valuefunction to resolve and aggregate ERC20 values from claims.getERC20Value.