Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Support overloaded ABI functions when invoking abigen macro #238

Closed
mattsse opened this issue Mar 18, 2021 · 1 comment · Fixed by #501
Closed

Support overloaded ABI functions when invoking abigen macro #238

mattsse opened this issue Mar 18, 2021 · 1 comment · Fixed by #501
Labels
enhancement New feature or request

Comments

@mattsse
Copy link
Collaborator

mattsse commented Mar 18, 2021

Is your feature request related to a problem? Please describe.

I tried to replace the auto-generated code in the DsProxyFactory and came accross an issue in the abigen output
The DSProxyFactory-ABI has an overloaded build function which gets translated to following rust code:

///Calls the contract's `build` (0x8e1a55fc) function
pub fn build(&self) -> ContractCall<M, Address> {
    self.0
        .method_hash([142, 26, 85, 252], ())
        .expect("method not found (this should never happen)")
}
///Calls the contract's `build` (0xf3701da2) function
pub fn build(&self, owner: Address) -> ContractCall<M, Address> {
    self.0
        .method_hash([243, 112, 29, 162], owner)
        .expect("method not found (this should never happen)")

Describe the solution you'd like

In case of overloaded functions we would follow rust's general convention of suffixing the function name with _with
The first function or the function with the least amount of arguments should be named as in the ABI, the following functions suffixed with _with_ + additional_params[0].name + (_and_(additional_params[1+i].name))*

The code above then would look like:

///Calls the contract's `build` (0x8e1a55fc) function
pub fn build(&self) -> ContractCall<M, Address> {
    self.0
        .method_hash([142, 26, 85, 252], ())
        .expect("method not found (this should never happen)")
}
///Calls the contract's `build` (0xf3701da2) function
pub fn build_with_owner(&self, owner: Address) -> ContractCall<M, Address> {
    self.0
        .method_hash([243, 112, 29, 162], owner)
        .expect("method not found (this should never happen)")

EDIT: just realized this can be mitigated with the method aliases in the macro, so this might be a non-issue

@mattsse mattsse changed the title Support overloaded function when invoking abigen macro Support overloaded ABI functions when invoking abigen macro Mar 18, 2021
@gakonst
Copy link
Owner

gakonst commented Mar 19, 2021

Yes, the method alias does it, but the UX of using it subpar and IIRC the errors are very overwhelming.

I am supportive of automatically appending _with_...! @prestwich seems to agree that this handled by the methods override, but the UX could be improved, so I'd say go for it, if it's easy to implement! Thanks again.

@gakonst gakonst added the enhancement New feature or request label Mar 19, 2021
meetmangukiya pushed a commit to meetmangukiya/ethers-rs that referenced this issue Mar 21, 2022
meetmangukiya pushed a commit to meetmangukiya/ethers-rs that referenced this issue Mar 21, 2022
* add expectRevert

* fmt

* clippy

* update readme

* typos

* feat: bump ethers for new ethabi types (gakonst#238)

ref: gakonst#700

* custom revert test

* use ethabi to decode

* PR recs

* fmt

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants