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.

Sign up for GitHub

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

Jump to bottom

feat: implement load_abi util function #6

Merged
merged 4 commits into from
Oct 18, 2024
Merged

feat: implement load_abi util function #6

merged 4 commits into from
Oct 18, 2024

Conversation

tamaralipows
Copy link
Contributor

@tamaralipows tamaralipows commented Oct 16, 2024 •
edited
Loading

@tamaralipows tamaralipows force-pushed the tnl/ENG-3749-load-abi branch from 3fee6bb to c4ea3a0 Compare October 17, 2024 14:44
kayibal
kayibal reviewed Oct 17, 2024
View reviewed changes
kayibal
kayibal reviewed Oct 17, 2024
View reviewed changes
@tamaralipows tamaralipows force-pushed the tnl/ENG-3749-load-abi branch from c4ea3a0 to ee81b3c Compare October 17, 2024 15:16
fix: split into load_erc20_abi and load_swap_abi
57d3415 
It's not necessary to be able to load any abi we want to. These are the only two relevant abis at this point.
@tamaralipows tamaralipows marked this pull request as ready for review October 17, 2024 16:01
louise-poole
louise-poole reviewed Oct 18, 2024
View reviewed changes
Copy link
Contributor

@louise-poole louise-poole left a comment

Choose a reason for hiding this comment

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

Much better! I have a few further suggestions:

fix: return ethers::ebi::Abi from load_swap_abi
2c18f68 
This will make it much easier to use from within rust since it will already be compatible with the ethers library.

Other cleanups:
- Don't duplicate ABI assets
- Remove unnecessary comment, add spacing btw tests.
louise-poole
louise-poole approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@louise-poole louise-poole left a comment

Choose a reason for hiding this comment

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

LGTM!

@tamaralipows tamaralipows merged commit d7a0498 into main Oct 18, 2024
3 checks passed
@propellerci
Copy link

propellerci bot commented Oct 18, 2024

This PR is included in version 0.23.0 🎉

@propellerci propellerci bot added the true label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kayibal kayibal kayibal left review comments

@louise-poole louise-poole louise-poole approved these changes

Assignees
No one assigned
Labels
true
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tamaralipows @kayibal @louise-poole