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

Improve binding generation #2605

Closed
fulminmaxi opened this issue Aug 4, 2022 · 4 comments
Closed

Improve binding generation #2605

fulminmaxi opened this issue Aug 4, 2022 · 4 comments
Labels
C-forge Command: forge Cmd-forge-bind Command: forge bind T-feature Type: feature

Comments

@fulminmaxi
Copy link

Component

Forge

Describe the feature you would like

My team has been using foundry and forge bindings in production for a few months, they're great but have a few rough edges, here is some feedback to make binding generation even better, in order of importance (for us at least):

  • Provide a way to filter out tests, scripts and library related bindings. Today forge bind generates bindings for all contracts, and some of them (like bindings to forge-std contacts) are useless and add time to the build. In our repo, we delete 30 files and usually leave 2/3 (our actual contracts)

  • Re-compile contracts by default. I personally shoot myself in the foot a few times by not recompiling before publishing our bindings, its easy to forget as all other forge commands re-compile before running.

  • Use a fixed ethers-rs version instead of the latest one from github

Additional context

No response

@fulminmaxi fulminmaxi added the T-feature Type: feature label Aug 4, 2022
@gakonst gakonst added this to Foundry Aug 4, 2022
@gakonst gakonst moved this to Todo in Foundry Aug 4, 2022
@mattsse
Copy link
Member

mattsse commented Aug 4, 2022

Use a fixed ethers-rs version instead of the latest one from github

could you elaborate on why this is more desirable? Do you need it for publishing your crate?

the reason we do this is, is because we're still adding new stuff and constantly fix things and it can take some time until a new release lands on crates.io

so unless you want to publish the project, imo it's preferable to depend on master and use the lock file.

@onbjerg onbjerg added C-forge Command: forge Cmd-forge-bind Command: forge bind labels Aug 4, 2022
@gakonst
Copy link
Member

gakonst commented Aug 4, 2022

could you elaborate on why this is more desirable? Do you need it for publishing your crate?

i think we just forgot to bump the ethers version in the abigen'ed code

@fulminmaxi
Copy link
Author

Use a fixed ethers-rs version instead of the latest one from github

could you elaborate on why this is more desirable? Do you need it for publishing your crate?

the reason we do this is, is because we're still adding new stuff and constantly fix things and it can take some time until a new release lands on crates.io

so unless you want to publish the project, imo it's preferable to depend on master and use the lock file.

Thanks for the super quick reply! We do publish the bindings to our internal cargo registry and use them in other repos (which I understand might be unusual), and I don't have a compelling argument for people building single repo apps, so its probably better to leave it as is

@onbjerg
Copy link
Member

onbjerg commented Aug 11, 2022

Fixed in the linked PRs above.

@onbjerg onbjerg closed this as completed Aug 11, 2022
Repository owner moved this from Todo to Done in Foundry Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-bind Command: forge bind T-feature Type: feature
Projects
Archived in project
Development

No branches or pull requests

4 participants