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

Updated legacy account type to Klaytn account type #1154

Merged
merged 6 commits into from
Feb 11, 2022

Conversation

jeongkyun-oh
Copy link
Contributor

@jeongkyun-oh jeongkyun-oh commented Feb 9, 2022

Proposed changes

  • Ethereum account type is removed and substitute them with KIaytn account type
  • Ethereum account try to save storage, so it implemented SlimAccount type which substitute emptyHash and emptyCodeHash to nil value. In this PR, the SlimAccount concept is deleted since we have two different types of Account.
  • Simply put, we do encoding/decoding account.Account type.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

@jeongkyun-oh jeongkyun-oh self-assigned this Feb 9, 2022
@jeongkyun-oh jeongkyun-oh changed the title Update eth account Update Legacy account type to Klaytn account type Feb 9, 2022
@jeongkyun-oh jeongkyun-oh marked this pull request as ready for review February 9, 2022 23:36
snapshot/generate.go Outdated Show resolved Hide resolved
snapshot/generate_test.go Outdated Show resolved Hide resolved
@jeongkyun-oh jeongkyun-oh changed the title Update Legacy account type to Klaytn account type Updated legacy account type to Klaytn account type Feb 10, 2022
Comment on lines 657 to 661
contractAcc, ok := acc.(*account.SmartContractAccount)
if !ok {
accMarker = nil
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to call wipeKeyRange for EOA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aidan-kwon Can you check this again?

@aidan-kwon
Copy link
Member

aidan-kwon commented Feb 11, 2022

@KimKyungup @ehnuje PTAL this Snapshot PR. We need one more approve

@jeongkyun-oh jeongkyun-oh merged commit b5d1e53 into klaytn:dev Feb 11, 2022
@jeongkyun-oh jeongkyun-oh deleted the update-eth-account branch February 11, 2022 05:45
Comment on lines -632 to -637
if bytes.Equal(acc.CodeHash, emptyCode[:]) {
dataLen -= 32
}
if acc.Root == emptyRoot {
dataLen -= 32
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this logic? Is it unnecessary in Klaytn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hqjang-pepper This is for approximate data size. So, Etheruem uses slim account in order to save the space. It reduces dataLen since nil is stored instead of emptyRootHash or emptyCodeHash.

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.

None yet

5 participants