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

add erc20 compatible allowances #950

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gregdhill
Copy link
Contributor

I'm not sure if this has been considered before, but we are working on adding ERC20 precompiles here and we need some way to approve the spending of funds. Substrate already supports this in pallet-assets so we can follow their design approach. For instance they do require deposits which I have currently omitted from this PR for simplicity but let me know if that is something you'd like to see. Otherwise the current change itself is trivial, we have another storage map which tracks the total amount approved for spending which can be updated either by the set_allowance extrinsic or it will decreased when the spender calls transfer_allowance.

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #950 (b765fd2) into master (7e15fcf) will decrease coverage by 0.03%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master     #950      +/-   ##
==========================================
- Coverage   78.46%   78.43%   -0.03%     
==========================================
  Files         105      105              
  Lines       10819    10879      +60     
==========================================
+ Hits         8489     8533      +44     
- Misses       2330     2346      +16     
Files Changed Coverage Δ
tokens/src/weights.rs 0.00% <0.00%> (ø)
tokens/src/lib.rs 77.14% <75.75%> (-0.05%) ⬇️
tokens/src/tests.rs 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xlc
Copy link
Member

xlc commented Sep 2, 2023

I just don't like the approve/transferFrom model. I understand we need it for ERC20 but I don't want to expose them in pallets. There is no reason user should use those calls and therefore we shouldn't have those calls.

There are two alternative approach:
Something like Acala does, use EVM contracts to implement those. e.g. https://github.com/AcalaNetwork/predeploy-contracts/blob/master/contracts/token/Token.sol#L72
Implement them in a different pallet so other orml-tokens user don't need to have those code.

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