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.

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

feat(contracts): Port contracts from ethers-rs #8

Merged
merged 14 commits into from
Mar 21, 2024

Conversation

zerosnacks
Copy link
Member

@zerosnacks zerosnacks commented Mar 15, 2024

Motivation

To add examples from ethers-rs: contracts

Solution

Implements a subset of the examples in ethers-rs as they are largely already covered by other examples or the implementation was missing. I've made some modifications to make it better fit for Alloy.

I have not included the contract compilation flow as Alloy does not re-export solc.

Blocked

Marked as blocked until alloy-rs/alloy#326 is merged

examples/contracts/examples/deploy_from_artifact.rs Outdated Show resolved Hide resolved
Comment on lines 42 to 45
let contract_builder = Counter::deploy_builder(&provider);
let estimate = contract_builder.estimate_gas().await?;
let contract_address =
contract_builder.gas(estimate).gas_price(base_fee).nonce(U64::from(0)).deploy().await?;
Copy link
Member

Choose a reason for hiding this comment

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

these should be auto-calculated, probably part of @yash-atreya's layer in alloy-rs/alloy#326 which i think probably should be always on, and we add an method to not layer it in the builder

Copy link
Member Author

Choose a reason for hiding this comment

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

Will implement in follow up ticket: #12

examples/contracts/examples/deploy_from_artifact.rs Outdated Show resolved Hide resolved
Comment on lines +75 to +76
// Retrieve the number, which should be 43.
let Counter::numberReturn { _0 } = contract.number().call().await?;
Copy link
Member

Choose a reason for hiding this comment

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

@DaniPopes @prestwich should we make the return value be automatically unwrapped/returned here by number().call() if it's a single field? i.e. i would pref to do let ret = contract.number().call().await?;

Copy link
Member

Choose a reason for hiding this comment

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

That'd be an inconsistent user-facing API, which I'm not generally a fan of

Copy link
Member

@gakonst gakonst Mar 16, 2024

Choose a reason for hiding this comment

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

Yes but this API is not good either, let Counter::numberReturn { _0 } :( I could see us supporting deref to the inner field otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Yes but this API is not good either, let Counter::numberReturn { _0 } :( I could see us supporting deref to the inner field otherwise.

changing it to a single unwrapped ret value will also break symmetry of encode/decode for dynamically sized ret values, as i believe MyStruct and (MyStruct,) may have different encodings in some cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to be merging this ticket as is, created this ticket: https://github.com/alloy-rs/alloy/issues/357 for the conversation to continue

let contract = IERC20::new("0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2".parse()?, provider);

// Call the contract, retrieve the total supply.
let IERC20::totalSupplyReturn { _0 } = contract.totalSupply().call().await?;
Copy link
Member

Choose a reason for hiding this comment

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

still think we should do snake_case and add serde-renames style attributes to the sol macro @DaniPopes @prestwich

Copy link
Member

Choose a reason for hiding this comment

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

before we add any more features to sol! we need to refactor to break out the input and expanders to make it more maintainable

Copy link
Member Author

Choose a reason for hiding this comment

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

Created a ticket here: https://github.com/alloy-rs/alloy/issues/357 to continue the conversation

Going to merge this ticket as is - will reflect any changes in Alloy here

@zerosnacks zerosnacks self-assigned this Mar 18, 2024
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

@zerosnacks
Copy link
Member Author

Going to merge this, future API enhancements not yet reflected in Alloy are described in tickets here: alloy-rs/core#580 + alloy-rs/core#581

@zerosnacks zerosnacks merged commit 364eafe into main Mar 21, 2024
2 checks passed
@zerosnacks zerosnacks deleted the zerosnacks/contracts branch March 21, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants