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

core, light, trie: polish code #303

Conversation

rjl493456442
Copy link

No description provided.

@rjl493456442 rjl493456442 requested a review from holiman as a code owner October 30, 2023 08:54
@rjl493456442 rjl493456442 force-pushed the geth-init-with-verkle-pbss-2 branch from c632a29 to 93e7504 Compare October 30, 2023 08:58
@rjl493456442 rjl493456442 force-pushed the geth-init-with-verkle-pbss-2 branch from 93e7504 to 037a03d Compare October 30, 2023 09:01
Copy link
Owner

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Thanks for doing all of that. I would like to keep the structure and especially the code locations/function names closer to the original names because there is a lot more stuff to merge and some functions' new names are misleading in my opinion. I agree on the GetStem / InsertStem issue that I described below, I will create a PR for that.

core/state/database.go Outdated Show resolved Hide resolved
trie/verkle/utils.go Outdated Show resolved Hide resolved
trie/verkle/utils.go Outdated Show resolved Hide resolved
trie/verkle.go Outdated Show resolved Hide resolved
trie/verkle.go Outdated Show resolved Hide resolved
trie/verkle.go Outdated Show resolved Hide resolved
trie/verkle.go Show resolved Hide resolved

// Get returns the cached commitment for the specified address, or computing
// it on the flight.
func (c *Cache) Get(addr []byte) *verkle.Point {
Copy link
Owner

Choose a reason for hiding this comment

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

the initial name was GetTreeKeyHeader. It gets keys in the account header, and so needs nothing more than the account address. Get is too generic and gives the impression that this key would return any key for the account, which isn't the case.

Copy link
Author

Choose a reason for hiding this comment

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

Well, I understand it in a different perspective. The cache is for storing the evaluated curve point. The address is the input for computing the curve point and Get is used to retrieve the corresponding point. The benefit for naming it as Get is because the function name is shorter and cleaner.

@rjl493456442
Copy link
Author

Please take another look, I know the point that we want to minimize the conflicts and I also hope it, I will change accordingly.

@gballet
Copy link
Owner

gballet commented Oct 31, 2023

I wasn't clear: I am fine with GetStem so I renamed InsertStem/GetStem to InsertValuesAtStem/GetValuesAtStem. It's for the other GetKey* replaced by Get that I have an issue.

Copy link
Owner

@gballet gballet left a comment

Choose a reason for hiding this comment

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

LGTM

@gballet gballet merged commit d326ba1 into gballet:geth-init-with-verkle Oct 31, 2023
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.

2 participants