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

chore(contracts): Add binding check #1217

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ethenotethan
Copy link
Contributor

@ethenotethan ethenotethan commented Feb 5, 2025

Why are these changes needed?

Adds a hygiene workflow which ensures that abi artifacts must be reflective of latest contract changes. Also noticed we were using two compilation commands (compile-el, compile-dl); these don't appear to have any instrumentation over the compile.sh script behavior so condensed into single compile-contracts target 😄

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@@ -37,3 +37,31 @@ jobs:
- name: Run snapshot
run: forge snapshot
working-directory: ./contracts

binding-verify:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this run in CI then?

If so, why have the bindings committed to the repo at all, if we generate them in CI?

Copy link
Contributor

Choose a reason for hiding this comment

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

the CI is just recreating the bindings and making sure that no new artifacts are created (by making sure git status is clean). If it fails it reminds the PR creator that they forgot to update the bindings

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not something to address for this PR, but my point is that I don't think we should have generated files checked into git at all. CI should generate, and local builds should also generate. Tracking via git is just clutter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@litt3 How would package artifacts be importable under this model? iiuc this would make EigenDA entirely self contained. Also generating them once ensures that everyone is pinned to the same code.

Copy link
Contributor

Choose a reason for hiding this comment

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

How would package artifacts be importable under this model?

The files would be generated as part of the build process- so they would be importable just like they currently are, after doing a build. And if any local changes were to be made to the contracts, then the locally generated artifacts would be updated during the next build execution.

Also generating them once ensures that everyone is pinned to the same code.

Is the concern here that the artifact generation might not be deterministic? If we do trust the determinism of the generation, then we are guaranteed that everyone is using the same generated artifacts, as long as they are using the same source files.

I suppose committing the generated files to git provides a check that the CI artifact generation precisely matches the artifact generation of the development machine used to commit the files to git. This doesn't seem like the most robust mechanism to test deterministic generation, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it useful to view generate golang code, e.g. you could see clearly what's the function signature in golang is like when making a call.

Copy link
Contributor

Choose a reason for hiding this comment

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

With my proposed strategy, you would still be able to view the generated golang code. Everything would be exactly the same on disk, the only thing that would change is what's committed to git

Copy link
Contributor

Choose a reason for hiding this comment

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

The use case is code viewing on github

@jianoaix
Copy link
Contributor

jianoaix commented Feb 5, 2025

The PR lgtm, but i think we had some discussion before regarding if requiring the latest version of binding, if anyone remembers it, may bring the context here so we make sure this is what we decided to go

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