-
-
Notifications
You must be signed in to change notification settings - Fork 65
added support for PERMIT2_TRANSFER_FROM_BATCH, closes #106 #128
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
base: master
Are you sure you want to change the base?
Conversation
|
@Elnaril any input or improvement for this pr? |
|
Hi @SOGeKING-NUL |
|
would like some help with that, what changes would I need to make for the CI checks to pass? do I need to tinker with some other files for this? |
|
@Elnaril fixed the issue, please review pr now :) |
Elnaril
left a comment
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.
Hey @SOGeKING-NUL , thanks for this nice PR! :) The core is spot-on and well commented. 👍
Having said that, there are some missing parts:
- I'd like to add the decoding of an actual mainnet transaction in
test_decoder.py::test_decode_transaction(let me know if you can't find one) - Also an integration test should be added or modified to demonstrate the implementation of this new function works as expected.
Also, please have a look to my other comments.
|
noted, will work on changes |
|
@Elnaril does this solve a lot of the issues? also could you tell me more about adding the decoding of an actual mainnet transaction |
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.
@SOGeKING-NUL Yes, it solved all the code issues and the coverage. 👍
also could you tell me more about adding the decoding of an actual mainnet transaction
The idea is to find a transaction which:
- happened on Ethereum Mainnet
- called
PERMIT2_TRANSFER_FROM_BATCH - ideally performing more than one transfer using this function
- decode it in
test_decoder.py::test_decode_transaction
To find such transaction, you could either use Etherscan or figure out a programmatic approach.
Does it make sense ?
Apart from that, the remaining tasks are:
- integration test
- squash commits (so 1 PR =1 commit before I merge it)
|
@SOGeKING-NUL |
|
Finally found one ! 😅 |


Summary
closes #106
Implementation Details
Enums: Added
RouterFunction.PERMIT2_TRANSFER_FROM_BATCH = 13ABI:
_build_permit2_transfer_from_batch()producing a single parameter:AllowanceTransferDetails[]address,address,uint256,address)build_abi_mapEncoder:
permit2_transfer_from_batch(transfer_details: List[Dict])Tests:
USDC+WETH)Screenshot