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

Remove non-standard increase_allowance and decrease_allowance from ERC20 #881

Merged

Conversation

TAdev0
Copy link
Contributor

@TAdev0 TAdev0 commented Jan 30, 2024

Fixes #728

This PR removes increase_allowance and decrease_allowance functions in ERC20 contracts (token and mocks)

PR Checklist

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

@TAdev0
Copy link
Contributor Author

TAdev0 commented Jan 31, 2024

@martriay oh yes I still need to modify the doc , I forgot to do it. Same for the other PR.

@TAdev0
Copy link
Contributor Author

TAdev0 commented Jan 31, 2024

@martriay now it should be good :)

@martriay
Copy link
Contributor

Thanks! Please complete all items in the PR checklist.

@TAdev0
Copy link
Contributor Author

TAdev0 commented Jan 31, 2024

@martriay am i suppose to ''try the feature'' on a public network? If yes what should i do exactly?

@martriay
Copy link
Contributor

Since this is a feature removal PR, we need to deploy the ERC20 preset and verify:

  • it works properly for regular operations
  • removed features don't work

Note that this PR modifies a preset, so we need to update the class hashes as indicated in the CONTRIBUTING guide.

@TAdev0
Copy link
Contributor Author

TAdev0 commented Jan 31, 2024

Thank very much for all explanations 🙏

@TAdev0
Copy link
Contributor Author

TAdev0 commented Feb 1, 2024

@martriay

Class hash: 0x7c9b5a246fe83b0cd81de65daddb94b3803f94950db99bf2431bbfecf284642

ERC20 preset deployed on Goerli at: 0x1ceea6ba0ad2015c13cdca03185f615127523d8ece2293975354163479db3e9

If you try to run, for example :

sncast --url https://free-rpc.nethermind.io/goerli-juno --account [your_account] --contract-address 0x1ceea6ba0ad2015c13cdca03185f615127523d8ece2293975354163479db3e9 --function "increase_allowance" --calldata 0x0 0x1 0x0

You will get the following error:

command: invoke
error: Transaction execution error = TransactionExecutionErrorData { transaction_index: 0, execution_error: "reverted: Error in the called contract (0x039ccf2f422488ac967891266df98cbf293e6dcafe6cf42ba603d2821fd9e247):\nError at pc=0:81:\nGot an exception while executing a hint: Custom Hint Error: Entry point EntryPointSelector(StarkFelt(\"0x01d13ab0a76d7407b1d5faccd4b3d8a9efe42f3d3c21766431d4fafb30f45bd4\")) not found in contract.\nCairo traceback (most recent call last):\nUnknown location (pc=0:731)\nUnknown location (pc=0:677)\nUnknown location (pc=0:291)\nUnknown location (pc=0:314)\n" }

This means `increase_allowance' entrypoint doesn't exist anymore.

Same applies for

sncast --url https://free-rpc.nethermind.io/goerli-juno --account [your_account] --contract-address 0x1ceea6ba0ad2015c13cdca03185f615127523d8ece2293975354163479db3e9 --function "decrease_allowance" --calldata 0x0 0x0 0x0
error: Transaction execution error = TransactionExecutionErrorData { transaction_index: 0, execution_error: "reverted: Error in the called contract (0x039ccf2f422488ac967891266df98cbf293e6dcafe6cf42ba603d2821fd9e247):\nError at pc=0:81:\nGot an exception while executing a hint: Custom Hint Error: Entry point EntryPointSelector(StarkFelt(\"0x03b076186c19fe96221e4dfacd40c519f612eae02e0555e4e115a2a6cf2f1c1f\")) not found in contract.\nCairo traceback (most recent call last):\nUnknown location (pc=0:731)\nUnknown location (pc=0:677)\nUnknown location (pc=0:291)\nUnknown location (pc=0:314)\n" }

Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Hey, thanks for opening this PR! I left a few comments and suggestions. Would you mind also removing the safe allowance impls from index.adoc here?

.tool-versions Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc20.adoc Show resolved Hide resolved
docs/modules/ROOT/pages/interfaces.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/utils/_class_hashes.adoc Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@TAdev0
Copy link
Contributor Author

TAdev0 commented Feb 2, 2024

@andrew-fleming PR updated, now it should be good!

Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

Looking good to me!

docs/modules/ROOT/pages/erc20.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/interfaces.adoc Outdated Show resolved Hide resolved
@TAdev0
Copy link
Contributor Author

TAdev0 commented Feb 6, 2024

@martriay docs updated!

Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

LGTM!

@martriay
Copy link
Contributor

martriay commented Feb 6, 2024

@TAdev0 please fix the merge conflicts and this is good to go.

@TAdev0
Copy link
Contributor Author

TAdev0 commented Feb 6, 2024

@martriay now it should be good!

@martriay martriay merged commit 2815498 into OpenZeppelin:main Feb 6, 2024
5 checks passed
@martriay
Copy link
Contributor

martriay commented Feb 6, 2024

Thanks!

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.

Remove non-standard increase_allowance and decrease_allowance from ERC20
4 participants