Skip to content

Conversation

@JonathanOppenheimer
Copy link
Member

@JonathanOppenheimer JonathanOppenheimer commented Dec 8, 2025

Why this should be merged

Currently, the committed ABI files are 2 years old, and there is no clear path of how they were generated. This PR deletes those files, and commits the files generated by compile.go instead.

You can verify that each folders contract.abi files matches the newly commited ABI file through the following:

  1. pretty format the new ABI through https://jsonformatter.org/
  2. diff check

You can also compare them directly using json, e.g. <(git show HEAD:precompile/allowlist/allowlist.abi | jq -S .) <(cat precompile/allowlist/IAllowList.abi | jq -S .) && echo "Files are identical!"

There are two exceptions which are commented below:

How this was tested

CI

Need to be documented?

No

Need to update RELEASES.md?

No

…nathanOppenheimer/convert-reward-manager-test

Signed-off-by: Jonathan Oppenheimer <[email protected]>
@JonathanOppenheimer JonathanOppenheimer removed the request for review from maru-ava December 8, 2025 22:21
@ARR4N
Copy link
Contributor

ARR4N commented Dec 9, 2025

ABIs are not a part of Solidity, they're generic. Even though we used Solidity to generate those, the starting point for a stateful precompile starts with the ABI not the Solidity. So the ABI is really the defining point for the stateful precompiles (thus why we have them in the prod code).

While this is true, any smart contract that needs to call the precompile needs the Solidity interface, so there must be a source of truth that defines both the precompile and the way it's called.

@michaelkaplan13
Copy link
Contributor

Please generate them in place to begin with so we can just review directly without depending on something beyond the scope of the PR. Next you can create a trivial PR that moves them.

If you check the commit I just added (a8e82e8), I can not get git to detect the similarity because the old file is formatted prettily and thus is 100+ lines. The newly generated file is a single line. Even setting the git similarity score to 1% on commit is not enough to track that these files are identical.

You can compare the files as JSON <(git show HEAD:precompile/allowlist/allowlist.abi | jq -S .) <(cat precompile/allowlist/IAllowList.abi | jq -S .) && echo "Files are identical!" to verify that these files are identical.

if you have any better suggestions, please let me know.

Following up a few different threads here.

  1. I just created a draft PR into this branch that has the ABI files generated in place and keeps the pretty-json formatting, which reduces the diff (particularly when viewed in GitHub). @JonathanOppenheimer, please take a look and make sure it makes sense to you. There were setRole events added to a few of the ABI files that are worth investigating.
  2. The blocker for moving to a monorepo in AvalancheGo was/is the removal of NPM from this repo. The need for this change came out of reviewing PRs that worked towards removing NPM, but this PR itself should not be considered a blocker. For that reason, lets defer it to after the monorepo.
  3. We definitely still want to make this change for (at least) the following reasons:
    a. Include programatic method for how the ABI files are generated
    b. Ensure via CI both that the ABI files are up to date with the the solidity interface definitions, and also that they are up to date with the precompile implementations (by using them to interact with the precompiles).

@JonathanOppenheimer
Copy link
Member Author

JonathanOppenheimer commented Dec 10, 2025

I have merged @michaelkaplan13's PR into this, and will leave the contract.abi rename for the separate cleanup PR as requested.

Base automatically changed from JonathanOppenheimer/delete-npm to master December 12, 2025 15:55
@JonathanOppenheimer JonathanOppenheimer removed the DO NOT MERGE This PR is not meant to be merged in its current state label Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing This primarly focuses on testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants