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

refactor: align accounts/abi/bind with coreth+upstream #1427

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

Conversation

darioush
Copy link
Collaborator

@darioush darioush commented Jan 22, 2025

Includes changes to the accounts/abi/bind package, focusing on renaming types and functions for consistency with upstream and coreth by moving some functionality to a new file.

The most important changes include renaming TmplContract, TmplMethod, and TmplStruct to tmplContract, tmplMethod, and tmplStruct, respectively, and moving the BindHook function and related types to a new file bind_extra.go.

This makes our modifications much clearer.

Some additional cosmetic changes are also reduced.

@darioush darioush changed the title reduces diffs with coreth refactor: align accounts/abi/bind with coreth+upstream Jan 22, 2025
@darioush darioush marked this pull request as ready for review January 22, 2025 01:17
@darioush darioush requested review from ceyonur and a team as code owners January 22, 2025 01:17
ceyonur
ceyonur previously approved these changes Jan 22, 2025
qdm12
qdm12 previously approved these changes Jan 24, 2025
Copy link
Collaborator

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

👍 Just a few minor comments

accounts/abi/bind/bind_extra.go Outdated Show resolved Hide resolved
accounts/abi/bind/bind_extra.go Outdated Show resolved Hide resolved
@@ -7,7 +7,6 @@
// original code from which it is derived.
//
// Much love to the original authors for their work.
// **********
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the removal? The geth files seem to have no copyright at all, so I'm not sure why we would modify this 🤔

Copy link
Collaborator Author

@darioush darioush Jan 24, 2025

Choose a reason for hiding this comment

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

to match coreth.

cmd/abigen/namefilter_test.go Show resolved Hide resolved
@@ -319,7 +319,7 @@ func (t *tester) generate(parent common.Hash) (common.Hash, *trienode.MergedNode
return root, ctx.nodes, triestate.New(ctx.accountOrigin, ctx.storageOrigin, nil)
}

// lastRoot returns the latest root hash, or empty if nothing is cached.
// lastHash returns the latest root hash, or empty if nothing is cached.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a typo in geth, I propose we revert this, however, if you want, I can be the typo comment man and accumulate these in a branch on the libevm repository, that we can PR to geth when it gets large enough?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Coreth already contains this typo fix, so I suppose why not then. Ideally we should revert the fix in coreth but that seems like a lot of trouble for just this.

Anyway, I opened ava-labs/libevm#108 feel free to commit to it when you find a geth code typo.

Copy link
Collaborator Author

@darioush darioush Jan 24, 2025

Choose a reason for hiding this comment

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

Same as above, here I'm just trying to reduce the difference in our repos.
We should try to take the geth changes as part of geth updates and fix them in upstream PRs.

Co-authored-by: Quentin McGaw <[email protected]>
Signed-off-by: Darioush Jalali <[email protected]>
@darioush darioush dismissed stale reviews from qdm12 and ceyonur via 5f35962 January 24, 2025 18:28
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.

3 participants