Skip to content
This repository was archived by the owner on Dec 16, 2025. It is now read-only.

Reduce ABI Gen diff#1916

Merged
JonathanOppenheimer merged 4 commits intoJonathanOppenheimer/use-new-abifrom
michaelkaplan13/reduce-abi-gen-diff
Dec 10, 2025
Merged

Reduce ABI Gen diff#1916
JonathanOppenheimer merged 4 commits intoJonathanOppenheimer/use-new-abifrom
michaelkaplan13/reduce-abi-gen-diff

Conversation

@michaelkaplan13
Copy link
Copy Markdown
Contributor

Inlines the ABI generation so that the existing files remain, and keeps the pretty-json formatting.

Also removes the metadata hashes from the bytecode of the compiles test contracts, so that it prevents unnecessary diffs in the future.

@michaelkaplan13
Copy link
Copy Markdown
Contributor Author

The resulting combined diff is best viewed by having this PR be based on the JonathanOppenheimer/delete-npm branch.

Copy link
Copy Markdown
Contributor

@JonathanOppenheimer JonathanOppenheimer left a comment

Choose a reason for hiding this comment

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

I like these changes -- the pretty print is a nice addition, and it makes reading the generated files easier for a human too.

Are you good if I merge this and then add a commit in the main branch to get rid of your renaming change? I think a whole bunch of contract.abis is unclear, and git should be able to show the move properly as a rename now anyway (so it'll be easy to review), and two files won't be shown, even if we rename allowlist.abi -> IAllowList.abi

@michaelkaplan13
Copy link
Copy Markdown
Contributor Author

I like these changes -- the pretty print is a nice addition, and it makes reading the generated files easier for a human too.

Are you good if I merge this and then add a commit in the main branch to get rid of your renaming change? I think a whole bunch of contract.abis is unclear, and git should be able to show the move properly as a rename now anyway (so it'll be easy to review), and two files won't be shown, even if we rename allowlist.abi -> IAllowList.abi

Definitely feel free to merge into your branch whenever. I think renaming the contract.abi files is best left for a separate PR though. It's not required, and it's best to be a standalone trivial PR in the future as Arran suggested.

@JonathanOppenheimer
Copy link
Copy Markdown
Contributor

Sounds good - I can do that in the future PR.

@JonathanOppenheimer JonathanOppenheimer marked this pull request as ready for review December 10, 2025 20:38
@JonathanOppenheimer JonathanOppenheimer requested a review from a team as a code owner December 10, 2025 20:38
@JonathanOppenheimer JonathanOppenheimer merged commit e9a4030 into JonathanOppenheimer/use-new-abi Dec 10, 2025
12 checks passed
@JonathanOppenheimer JonathanOppenheimer deleted the michaelkaplan13/reduce-abi-gen-diff branch December 10, 2025 20:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants