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

Add parity-codec cargo feature. #260

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

dandanlen
Copy link
Contributor

This is useful if you want to use ethabi in a substrate-related project, for example abi-encoding eth contract calls on-chain.

@vkgnosis
Copy link
Member

vkgnosis commented Feb 4, 2022

Since nothing in this crate changes with the feature, wouldn't it make more sense that you depend on ethereum-types/codec when you need the feature there rather than going through this crate?

@dandanlen
Copy link
Contributor Author

I could, but then I need to add an explicit dependency on ethereum-types, which is currently only a transitive dependency via ethabi.

For example, say I want to use Address type from ethabi. I don't really care that it is re-exported from ethereum-types, I just want it to be compatible with the ethabi crate.

I suppose I agree in that this feature would make more sense / would be more complete if it also conditionally derived the Encode and Decode traits for Function, Param etc.

Copy link
Member

@vkgnosis vkgnosis left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Will give the PR some time in case other people have an opinion.

Copy link
Contributor

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

LGTM

@gakonst
Copy link
Contributor

gakonst commented Feb 9, 2022

If you merge this, it'd also help to release a new version of the crate!

@nlordell nlordell merged commit 321a651 into rust-ethereum:master Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants