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

Inaccuracy in Approve Chain Extension behavior #281

Closed
ashneverdawn opened this issue Jul 18, 2023 · 1 comment · Fixed by #283
Closed

Inaccuracy in Approve Chain Extension behavior #281

ashneverdawn opened this issue Jul 18, 2023 · 1 comment · Fixed by #283
Assignees
Labels
priority:high Do it now

Comments

@ashneverdawn
Copy link
Contributor

ashneverdawn commented Jul 18, 2023

While implementing the weight charging for the token allowance pallet, I noticed that the approve behavior isn't consistent with OpenZeppelin's one.

1) The current behavior increments the approval, whereas it should overwrite.

See Docs: https://docs.openzeppelin.com/contracts/4.x/api/token/erc20#IERC20-approve-address-uint256-

Current behavior: https://github.com/pendulum-chain/pendulum/blob/0f6cbb5289d03bd85ab664e7724aba691fea8d81/pallets/orml-currencies-allowance-extension/src/lib.rs#L208C3-L208C3

Expected behavior: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/21bb89ef5bfc789b9333eb05e3ba2b7b284ac77c/contracts/token/ERC20/ERC20.sol#L345
Fixing this should also result in a lower gas cost when regenerating the weights. (no longer need to read before writing)

2) When approved for the max possible amount, transfer_from should treat it as infinity and not decrement it.

See Docs: https://docs.openzeppelin.com/contracts/4.x/api/token/erc20#ERC20-approve-address-uint256-

@ebma
Copy link
Member

ebma commented Jul 18, 2023

Hey team! Please add your planning poker estimate with Zenhub @ashneverdawn @TorstenStueber

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:high Do it now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants