-
Notifications
You must be signed in to change notification settings - Fork 14
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
Removal of x/authz dependency on precompiles #47
Comments
Hey, this generally looks good to me. To address each point individually:
|
@vladjdk Thanks for your feedback. As for my speculation regarding the x/authz dependency in other precompiles:
|
Internal Discussion SummaryTL;DR
1. BackgroundOur original roadmap included removing the 2. Use Cases and ExamplesEvmos provided several examples—such as simple-staker, simple-ibc-transfer, and flashloan—where a contract can be granted permission (via 3. Considering New Authz PrecompileTo preserve these use cases while still removing 4. Conclusion: No Separate Authz PrecompileAfter evaluating the potential security implications, we decided not to move forward with a separate
5. Next StepsWe will proceed with removing |
Modification of Staking Precompile Integration TestTL;DR
Test scenario changeCurrently, the integration tests for the staking precompile are largely divided into two parts:
Modifying the direct call approach is relatively simple, since it only requires removing the test cases that involve approve. The part that requires more extensive changes is the one using the Calls made through the Scenario 1Previous:
Updated:
Scenario 2Previous:
Updated:
|
I think payable is completely acceptable in this scenario. As far as I can tell, the current staking implementation falls under the assumption that the staking token should be treated as an ERC20. I think it makes total sense to have the staking implementation work with the native gas token instead. One question on the Lgtm :) |
@vladjdk If I may add, the Also, in both the current and updated test cases, staking is tested using the native gas token. “One question on the StakingCaller contract. It seems to only exist to test approvals and staking on behalf of the user.” |
Removal of x/authz dependency on precompiles
Summary
For erc20 precompile
Allowance
state and CRUD methods inx/erc20
module.Allowance
CRUD methods.For other precompiles
authzKeeper
field and related methods from common Precompile struct. [ref. Precompile.AuthzKeeper]Problems
1. Modularity of precompiles
2. Compatibility between ERC20 and authz spec
1 year
by default. However, When it is called thruogh x/auth module, it could be different value.SendAuthorization
through the authz module, those fields can be used. This may lead to cases where anallowance
exists, buttransferFrom
fails for certain addresses, or existing allowances disappear without any Transfer or Approval event being emitted.3. Unconfigurable Expiration of authz Grant
Solutions
For erc20 precompile
AllowancePrefix([]byte{4}) + TokenPairID + ownerAddr + spenderAddr
sdkmath.LegacyDec
For other modules
The text was updated successfully, but these errors were encountered: