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

Feature Request: Expand account compatible #55

Open
VictorTrustyDev opened this issue Jan 3, 2024 · 5 comments · May be fixed by #56
Open

Feature Request: Expand account compatible #55

VictorTrustyDev opened this issue Jan 3, 2024 · 5 comments · May be fixed by #56
Assignees
Labels
enhancement Enhancement / bug-fixes or re-work on old feature optimize Optimize business or code performance

Comments

@VictorTrustyDev
Copy link
Member

VictorTrustyDev commented Jan 3, 2024

Evermint is fork of Evmos v12.1.6 for research & development purpose

Update Sep 2024: implemented in Evermint by #129


Currently, Evermint is using EthAccount as the default proto account and the only type of account supported.
That causes some limit:

  • Use Cosmos-SDK x/vesting module
  • Limit potential of integrate new features from CosmosSDK
  • Wasting storage to store evmtypes.EmptyCodeHash 32 bytes value that also exists in every non-contract EthAccount (see prove).

Request:

  • Remove EthAccount
  • Make authtypes.BaseAccount as default proto account, compatible with EVM module.
  • All current testcases must be kept to make sure the implementation is fully compatible.
  • x/evm non-genesis account now only support export/import only accounts with either code or storage
  • Optimize store by only store code hash which len(code_hash) > 0 && code_hash <> evmtypes.EmptyCodeHash instead of saving it into every EthAccount record. This change will slightly lower gas fee required to read from store in non-evm Txs

Aware:

  • x/auth keeper RemoveAccount
  • Account bytes compatible: currently Cosmos supports 20 & 32 bytes address (ICA)

Mindset changes:

  • Contract account is now accounts that has CodeHash record (len(code_hash) == 0 || code_hash == evmtypes.EmptyCodeHash will be removed from store)
  • ETH-compatible account is now accounts those are not contract accounts, plus len(address_bytes) == 20. If it is in blockedAddrs by x/bank, will be an error in stateDb.Commit() phrase.
  • When doing account migration, remember to consider migrate code hash, if needed.

Migration aware (if any chain planned to implement this):

  • Must define migration to be able to upgrade Ethermint/Evmos chains.
  • Must test query historical data and data must be matched pre and after upgrade.
@VictorTrustyDev VictorTrustyDev added enhancement Enhancement / bug-fixes or re-work on old feature optimize Optimize business or code performance labels Jan 3, 2024
@VictorTrustyDev VictorTrustyDev self-assigned this Jan 3, 2024
@VictorTrustyDev
Copy link
Member Author

VictorTrustyDev commented Jan 3, 2024

Pieces of solution

  1. The legacy EthAccount stores code hash.
    => Introduce new store that store code hash.

  2. To be able to compatible with x/auth keeper::RemoveAccount, as well as a protection layer in case of code logic-if any, the new dedicated storages must has key is combination of address & account number. When an account is removed, the account and the account number will be removed from store so if the same account address is created again, with another account number, key pointer will refer to nil value. When contract is deleted, the key must be removed too. Noway an account can be created again with the same account number, except core team do it on purpose(?).

  3. Cosmos does support 20 bytes and 32 bytes address (like ICA) but code hash is only used to store contract's code hash so only 20 bytes address is considered. If any account with len(address_bytes) != 20 then it is absolutely not a contract.

Store:

New Code Hash store
Key-Prefix:

prefixCodeHash (iota=4)
KeyPrefixCodeHash = []byte{prefixCodeHash}

Key: 1 bytes of store key code hash prefix + 20 bytes of address + 8 bytes of account number Uint64ToBigEndian. To maintains absolute key length = 29 bytes, if bytes of account number is less than 8, must append prefix 0x00 until reached 8 bytes.

Test-cases:

  1. Store code hash:
  • Not contains the byte hash equals to empty code hash of non-contract account
  1. Multiple account types:
  • Other account types must be able to work with x/evm

@VictorTrustyDev VictorTrustyDev linked a pull request Jan 3, 2024 that will close this issue
@VictorTrustyDev
Copy link
Member Author

VictorTrustyDev commented Jan 3, 2024

Proof of Concept #56

I have made a branch to prove that the above solution that is, at this moment, seems to work perfectly with even all deprecated modules of Evmos.

15 commits for 15 implementation steps:

  1. remove EthAccount type
  2. define new store key for storing code hash
  3. remove registration of concrete interface
  4. introduce new type CodeHash for typed code & convenient method
  5. expand x/evm keeper with new methods to work with code hash & account type checking
  6. use default proto BaseAccount by x/auth
  7. migrate x/evm genesis account behavior
  8. genesis account is now base account
  9. update test with new account spec
  10. update statedb to use new code hash implementation
  11. update keeper to use new code hash implementation & account specification
  12. use BaseAccount instead of EthAccount in utils_test
  13. vesting now convert back to BaseAccount instead of EthAccount
  14. fix implementation & testcases for deprecated module x/claims
  15. fix implementation & testcases for deprecated module x/incentives

Notes:

  • StateDB::GetAccount: if code hash of account is nil or empty code hash, evmtypes.EmptyCodeHash will be returned along with the Account as usual.
  • StateDB::DeleteAccount minor changes: if account is not a contract account (read first post for condition), will be rejected with error. (Previously only check if it is EthAccount)
image

Test status:

  • Old testcases are kept.
  • I only run go test. One test failed, 2423 passed.
  • The failed testcase is should transfer tokens to the recipient and perform recovery from x/recovery module that I have no idea why failed (already failed pre-poc and I spent no time to investigate)

Migration (for existing chains) based on this implementation

  1. Migrate accounts:
  • Convert evmtypes.EthAccount back to authtypes.BaseAccount.
  • Call EVM keeper to store code hash of the removed EthAccount

@yihuang
Copy link

yihuang commented Jan 4, 2024

like the idea.

@fedekunze
Copy link

8 bytes of account number Uint64ToBigEndian

@VictorTrustyDev Why do you need the account number?

@VictorTrustyDev
Copy link
Member Author

VictorTrustyDev commented Apr 23, 2024

@fedekunze account destruction, account re-initialize.
I explain this in "Pieces of solution" point 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement / bug-fixes or re-work on old feature optimize Optimize business or code performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants