feat: remove x/authz dependency from precompiles #62
Conversation
cloudgray
changed the title
cloudgray
changed the title
There was a problem hiding this comment.
Looks good to me so far — the authz removal seems to be handled well.
I had one question while reviewing the test code, which I left as a comment.
Also, this is not directly related to the scope of your PR, but I noticed that some of the existing test cases don’t validate balance changes. For example, after a delete, there’s no assertion checking whether the delegator’s balance was reduced accordingly. Since the precompiled contract involves complex state transitions, I think it would be valuable to add more thorough state validation to ensure the transitions happen as expected.
This can probably be addressed in a separate PR, so just leaving this here for reference. I opened an issue to track it.
* 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
fix owner == sender case
| @@ -0,0 +1,164 @@ | |||
| package keeper | |||
There was a problem hiding this comment.
This comment is related to a change that wasn't mentioned in the review of the previous PR (#90), so I'm leaving it here.
With the addition of the allowance state to the ERC20 module, a state-breaking change has occurred.
However, it looks like the consensus version is still set to 4
There seem to be two possible options to address this:
- Increment the consensus version to
5— to maintain continuity with the version history used inevmos/os. - Reset the consensus version to
1— since we're starting fresh withcosmos/evm, setting it to the initial version (1) could also make sense.
If this isn't the appropriate PR to make a decision on this, I’d appreciate it if we could continue the discussion in a separate issue.
There was a problem hiding this comment.
I missed modifying consensus version of x/erc20 module. 🙏
I think consensus version should be 5 in this PR.
And resetting the consensus version to 1 is something that applies to other modules as well, so I think it should be handled in a separate issue. Because x/vm module and x/feemarket module also have consensus version from evmos/os [x/evm: 8] [x/feemarket: 5]
There was a problem hiding this comment.
Let's open a new issue for this yes
|
In the cosmos-evm context, naming convention is |
x/erc20/module.go
Outdated
| @@ -27,7 +27,7 @@ import ( | |||
| ) | |||
|
|
|||
| // consensusVersion defines the current x/erc20 module consensus version. | |||
| const consensusVersion = 4 | |||
| const consensusVersion = 5 | |||
There was a problem hiding this comment.
I think this work doesn't cause breaking changes, so you don't need to increment the consensus version.
Or, if this work cause breaking changes, you need to serve the migration code.
There was a problem hiding this comment.
Ok, I'll lower consensus version to before modification and open issue.
There was a problem hiding this comment.
I tested node starting and didn't cause breaking changes. test steps are below
1. start node with main branch
2. stop the node and build node on the `feat/remove-authz-from-precompiles` branch
3. start the node
4. no breaking changes.
if you agree with this test, you don't need to open the issues.
There was a problem hiding this comment.
I agree.
I think we should only open issue on whether initialize consensusVersion of x/modules to 1 or use current consensusVersion increased by evmos/evmos repo.
| return k.setAllowance(ctx, erc20, owner, spender, common.Big0, false) | ||
| } | ||
|
|
||
| func (k Keeper) setAllowance( |
There was a problem hiding this comment.
In my opinion, the setAllowance function should be defined purely as a function to set to store, and the other check logic should be left out as outer functions.
SetAllowance()
- check token pair
- call UnsafeSetAllowance
UnsafeSetAllowance
- check owner, spender, value, ...
- call setAllowance
setAllowance
- set allowance to store
i think this architecture looks so good to me.
There was a problem hiding this comment.
@dudong2 I mostly agree with your opinion.
But there is one problem.
If I check token pair external from UnsafeSetAllowance, checking existence of token pair in InitGenesis becomes difficult.
So, my proposal is as below.
SetAllowancecallsSetAllowanceWithValidationwith argallowDisabledTokenPair == falseSetAllowanceWithValidationtakes part of all validation and if allowDisabledTokenPair == true, it ignoresTokenPair.Enabled == falseand callsetAllowance.setAllowanceonly take part of setting store.
SetAllowance()
- call SetAllowanceWithValidation()
SetAllowanceWithValidation()
- check token pair
- **check enabled/disabled tokenPair (optionally)**
- check owner, spender, value, ...
- call setAllowance()
setAllowance()
- if value == nil, delete Allowance
- set allowance to storeThere was a problem hiding this comment.
applied in c39ffaa
If you think the change is inappropriate, I'll modify again.
There was a problem hiding this comment.
ok. Then I think below architecture is better.
SetAllowance()
- check token pair with enabled
- call setAllowance
UnsafeSetAllowance()
- check token pair w/o enabled
- call setAllowance
setAllowance()
- set allowance to store
There was a problem hiding this comment.
In your new design, I think check owner, spender, value, ... is omitted.
I think you mean check owner, spender, value, ... is performed in setAllowance().
Is it right?
There was a problem hiding this comment.
I modified as below for reducing logic redundancy. (aacfc4c)
SetAllowance()
- call setAllowance w/o allowDisabledTokenPair
DeleteAllowance()
- call setAllowance w/ allowDisabledTokenPair
UnsafeSetAllowance()
- call setAllowance w/ allowDisabledTokenPair
setAllowance()
- check token pair exists and enabled
- check owner, spender, value, ...
- set allowance to store
applied in dc225a2 |
Description
Removal of x/authz dependency on precompiles
Approve,Revoke,IncreaseAllowance,DecreaseAllowance,Allowance) and its usage.Allowancestate CRUD methods in x/erc20 moduleAllowancestate of x/erc20 module instead ofGrantstate of x/authz module for authorization methods (Approve,Revoke,IncreaseAllowance,DecreaseAllowance,Allowance)Closes: #47
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...
mainbranchReviewers 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...
Unreleasedsection inCHANGELOG.md