Skip to content

feat(x/erc20): add allowance state #90

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

Merged
merged 6 commits into from
Apr 21, 2025

Conversation

cloudgray
Copy link
Contributor

Description

  • Add Allowance state in x/erc20 module

Closes: #70


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the main branch

Reviewers Checklist

All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code
  • reviewed content
  • tested instructions (if applicable)
  • confirmed all CI checks have passed

Copy link
Contributor

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

@cloudgray
Thanks for the draft — I’ve left a few comments that might be helpful as you continue working on this.

owner = common.HexToAddress(allowance.Owner)
spender = common.HexToAddress(allowance.Spender)
value = allowance.Value.BigInt()
k.SetAllowance(ctx, erc20, owner, spender, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

@cloudgray
Could you add error handling here? If SetAllowance returns an error, we should panic to prevent inconsistent state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied in 80036a6

Copy link
Contributor

Choose a reason for hiding this comment

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

@cloudgray
It looks like disabled token pairs never get their allowances stored during InitGenesis, because SetAllowance calls checkTokenPair and returns an error if the token is disabled. However, InitGenesis is supposed to restore all state from the genesis file — including allowances for disabled pairs.

Right now, we simply continue when types.ErrERC20TokenPairDisabled is encountered, so those allowances don’t actually get written to the KVStore. If the module logic requires that disabled pairs still have their allowances recorded at genesis (so they might be re-enabled later or at least remain consistent with the imported state), we’ll need to revise this flow. One potential fix is to skip the checkTokenPair check during genesis, or handle the “disabled” error as a non-fatal condition that still writes the allowance. Otherwise, we lose a piece of state that was originally present in the genesis file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied in fcae5bb and 80036a6

@cloudgray
Copy link
Contributor Author

cloudgray commented Apr 18, 2025

@zsystm I added deleteAllowances method and let it called by DeleteTokenPair method.

It is not likely to happen that TokenPair for erc20 precompile is deleted and it really does not happen in cosmos/evm.
However, because DeleteTokenPair method is public method, appchain that imports cosmos/evm can call it in somewhere.
Then, when that appchain import genesis, the situation where TokenPair does not exists but allowances exist can happen.

So, I thought that all allowances that share same erc20 address are deleted when TokenPair is deleted.

I applied this change in 80036a6.
Could you check this change?

@cloudgray cloudgray marked this pull request as ready for review April 18, 2025 12:12
@cloudgray cloudgray requested a review from zsystm April 18, 2025 12:12
@zsystm
Copy link
Contributor

zsystm commented Apr 19, 2025

@zsystm I added deleteAllowances method and let it called by DeleteTokenPair method.

It is not likely to happen that TokenPair for erc20 precompile is deleted and it really does not happen in cosmos/evm. However, because DeleteTokenPair method is public method, appchain that imports cosmos/evm can call it in somewhere. Then, when that appchain import genesis, the situation where TokenPair does not exists but allowances exist can happen.

So, I thought that all allowances that share same erc20 address are deleted when TokenPair is deleted.

I applied this change in 80036a6. Could you check this change?

@cloudgray
I agree that allowances should be removed when a token pair is deleted — especially since the deletion happens when the paired ERC20 contract is self-destructed. In that case, keeping the allowance data serves no real purpose, so cleaning it up makes sense.

@zsystm
Copy link
Contributor

zsystm commented Apr 19, 2025

@cloudgray
I have one last comment from my side — please take a look when you get a chance: #90 (comment)

Copy link
Contributor

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

Final review from my side. Let's quickly resolve this and merge it.
Thanks!

Copy link
Contributor

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

LGTM. Let's squash & merge it.
For lint failures, we can handle this when open PR for merging feat into main.

@cloudgray cloudgray merged commit 9dd8c3d into feat/remove-authz-from-precompiles Apr 21, 2025
14 of 17 checks passed
@cloudgray cloudgray deleted the feat/erc20-allowance branch April 21, 2025 04:35
vladjdk added a commit that referenced this pull request May 9, 2025
* feat: remove authz dependency from precompiles except for erc20

* wip: feat(precompiles): remove x/authz dependency

* test(precompiles): remove x/authz dependent test cases

* feat: remove x/authz dependency from x/erc20 and werc20 precompile

* fix(precompiles): modify contract interface and test contracts

* fix(precompiles): remove unused authz related types and test utils

* fix(precompiles): modify integration test of staking precompile

* feat(x/erc20): add allowance state (#90)

* feat(x/erc20): add allowance state

* test(x/erc20): add unit test for Allowance

* fix(x/erc20): add deleteAllowances method and modify genesis validation

* fix(x/erc20): validation of SetAllowance/GetAllowance and InitGenesis

* test(x/erc20) add test cases for UnsafeSetAllowance

* chore: fix lint

* wip: test: modify erc20 precompile tests

* test: modify erc20 precompile tests

* test(precompiles): �fix integration test

fix owner == sender case

* refactor(precompiles): remove authz dependent naming and comments

* wip: test(x/vm): modify flashLoan test

* test(x/vm): fix flashLoan integration test

* fix: merge conflict

* chore: fix lint

* chore: fix lint

* feat: remove authz keeper dependency from x/erc20 keeper

* chore: fix lint

* fix: increase consensusVersion of x/erc20 module

* chore: udpate CHANGELOG.md

* chore: update CHANGELOG.md

* chore: add comments to StakingCaller solidity contract

* fix: typo

* fix: remove duplicated test case

* fix: wrong args for ApprovalEvent

* fix: wrong error message for test

* refactor: refine code for staking precompile

* fix: merge conflicts

* refactor: change file name

* refactor(testutil): move constants for test to testutil/constants package

* refactor(erc20): InitGenesis

* test(erc20): add genesis test case

* fix(erc20): revert consensusVersion change

* refactor(erc20): refactor keeper methods of Allowance state

* test(testutil): fix test case and lint

* wip: fix(precompiles/erc20): remove incompatible logic with ERC20 Specification

* fix: add Approval event to transferFrom method and fix integration tests

* fix: FlashLoan.sol contract

* fix(x/erc20): allowance methods and test cases

* test(precompiles): add test cases for erc20 precompile

---------

Co-authored-by: Vlad <[email protected]>
vladjdk added a commit that referenced this pull request May 26, 2025
* feat: remove authz dependency from precompiles except for erc20

* wip: feat(precompiles): remove x/authz dependency

* test(precompiles): remove x/authz dependent test cases

* feat: remove x/authz dependency from x/erc20 and werc20 precompile

* fix(precompiles): modify contract interface and test contracts

* fix(precompiles): remove unused authz related types and test utils

* fix(precompiles): modify integration test of staking precompile

* feat(x/erc20): add allowance state (#90)

* feat(x/erc20): add allowance state

* test(x/erc20): add unit test for Allowance

* fix(x/erc20): add deleteAllowances method and modify genesis validation

* fix(x/erc20): validation of SetAllowance/GetAllowance and InitGenesis

* test(x/erc20) add test cases for UnsafeSetAllowance

* chore: fix lint

* bump geth to v1.13

* remove gomod2nix file

* update comment

* wip: test: modify erc20 precompile tests

* test: modify erc20 precompile tests

* test(precompiles): �fix integration test

fix owner == sender case

* refactor(precompiles): remove authz dependent naming and comments

* wip: test(x/vm): modify flashLoan test

* test(x/vm): fix flashLoan integration test

* fix: merge conflict

* chore: fix lint

* chore: fix lint

* feat: remove authz keeper dependency from x/erc20 keeper

* chore: fix lint

* fix: increase consensusVersion of x/erc20 module

* chore: udpate CHANGELOG.md

* chore: update CHANGELOG.md

* chore: add comments to StakingCaller solidity contract

* fix govulncheck failure

* lint fix

* feat: x/precisebank module

* test: fix unit tests

* test: fix markdown lint

* test: apply gci for x/precisebank

* test: fix markdown lint

* test: fix lint

* feat: generalize decimals

* feat: add query cli

* feat: generalize conversion factor

* feat: add invariant about total supply

* chore: move config related files to evmd/config

* feat: store coin info (denom, conversion factor)

* chore: remove chain specific logic from x/vm/keeper/keeper.go

* test: fix lint

* chore: revert go.mod

* test: add fuzz test

* test: fix fuzz test

* test: fix test_find_solidity_files

* test: fix lint for python file

* feat: use precisebank module from precompile

* Revert "feat: store coin info (denom, conversion factor)"

This reverts commit b309bd9.

* chore: add ConversionFactorVal for genesis block

* chore: remove unused proto import

* test: add generalized decimal tests

* test: add random value test with multi decimals

* test: add contract interaction test

* feat: remove flooring logic

* Revert "feat: remove flooring logic"

This reverts commit 05aa469.

* feat: change logic from flooring to precisebank

* Revert "chore: move config related files to evmd/config"

* fix: node start

* refactor: bank wrapper in x/vm scaling codes

* test: fix lint

* chore: cache EVMCoin infos before ResetTestConfig

* fix: return from sendExtendedCoins, if from == to

* test: add SEED for deterministic random test

* test: add random evm coin transfer with random gas

* test: fix lint

* test: revert TestEvmAnteTestSuite test cases

* chore: revert useless changes

* chore: remove evmtype constant dependency from precisebank module

* chore: register precisebank query cli to AppModuleBasic

* test: remove useless configurator setting

* fix: build

* fix: lint

* feat: add extended denom to 18 decimals

* refactor: ConversionFactorVal -> Decimals

* chore: sdkmath -> math

* chore: revert useless changes

* chore: remove useless check for extended denom

* refactor: simplify GetBalance of BankWrapper

* test: restore removed test

* chore: remove useless decimal validation check

* chore: fix condition more clearly

* docs: CHANGELOG.md

* chore: use only precisebank module for evm module and precompile

* fix: typo

* fix: remove duplicated test case

* fix: wrong args for ApprovalEvent

* fix: wrong error message for test

* refactor: refine code for staking precompile

* update geth dep

* fix: merge conflicts

* chore: add extended denom for TestChainID

* refactor: change file name

* refactor(testutil): move constants for test to testutil/constants package

* refactor(erc20): InitGenesis

* test(erc20): add genesis test case

* fix(erc20): revert consensusVersion change

* refactor(erc20): refactor keeper methods of Allowance state

* test(testutil): fix test case and lint

* wip: fix(precompiles/erc20): remove incompatible logic with ERC20 Specification

* refactor: invariants

* chore: add comments for precisebank

* refactor: remove hardcoded values in precisebanktypes

* test: set evm coin info explicitly

* refactor: rename funcs

* chore: remove deprecated interface check

* docs: add thanks message to kava

* test: remove debug print

* chore: remove TotalFractionalBalances query

* fix: add Approval event to transferFrom method and fix integration tests

* fix: FlashLoan.sol contract

* fix(x/erc20): allowance methods and test cases

* refactor: rename funcs(ChangeEvmCoinDenomFrom18Decimals -> ConvertEvmCoinDenomToExtendedDenom)

* test: fix lint

* Revert "test: fix lint"

This reverts commit d546ec2.

* test: rename test names

* test(precompiles): add test cases for erc20 precompile

* test: add vesting test

* chore: remove GetAllBalances from bank keeper interface

* do dependency review regardless of diff

* fix: apply patch ISA-2025-004: Partial Precompile State Writes

* revert python3

* Add codecov and instructions

* append

* lint

* feat: eth_getBlockReceipts

* update to cosmos geth

* lints

* properly handle overflows on big int to uint256 conversion

* remove config from params on genesis

* safeguard gas refund overflows

* more lints

* remove migrations

* finish removing migrations

* remove migrations from module

* remove comments

* add todo for evm equivalence

* redo check

---------

Co-authored-by: Kyuhyeon Choi <[email protected]>
Co-authored-by: Haber <[email protected]>
Co-authored-by: tom <[email protected]>
Co-authored-by: dudong2 <[email protected]>
Co-authored-by: Suhail Kakar <[email protected]>
Co-authored-by: Tugay Emin <[email protected]>
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