Do not emit Approval event when calling transferFrom by Amxx · Pull Request #4370 · OpenZeppelin/openzeppelin-contracts (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits8 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fixes LIB-876
PR Checklist
- Tests
- Documentation
- Changeset entry (run
npx changeset add)
| const expectedSupply = initialSupply.add(amount); |
|---|
| expect(await this.token.totalSupply()).to.be.bignumber.equal(expectedSupply); |
| }); |
| const describeBurn = function (description, amount) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, I'd suggest:
| const describeBurn = function (description, amount) { |
|---|
| function describeBurn (description, amount) { |
Non-blocking.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not related to this PR. Maybe we can put that in a dedicated "cleanup/consistency" PR?
Otherwize we can do it here, I don't have a strong opinion.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't introduced in this PR so if we want to change it let's do it separately.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree in doing it in another PR
Co-authored-by: Ernesto García ernestognw@gmail.com
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Minor comment.
Amxx and others added 3 commits
Co-authored-by: Francisco fg@frang.io
frangio previously approved these changes Jun 21, 2023
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for just bringing it up but we talked before about registering the changes needed to migrate to 5.0 and I think this should go into this list.
Did we agree to put it in the CHANGELOG?
Co-authored-by: Ernesto García ernestognw@gmail.com
If there are any breaking changes that users need to adapt in their code when upgrading, we should put them in a section "How to Upgrade from 4.x" in the changelog.
In this case though users don't need to do anything, so I don't think there is anything we need to mention in that section, beyond what we should mention in the changeset.
ernestognw added a commit to ernestognw/openzeppelin-contracts that referenced this pull request
Co-authored-by: Ernesto García ernestognw@gmail.com Co-authored-by: Francisco fg@frang.io
ernestognw added a commit to ernestognw/openzeppelin-contracts that referenced this pull request
Co-authored-by: Ernesto García ernestognw@gmail.com Co-authored-by: Francisco fg@frang.io
This was referenced
Jun 15, 2024
This was referenced
Jul 9, 2024
This was referenced
Sep 10, 2024
This was referenced
Sep 12, 2024
This was referenced
Nov 8, 2024
This was referenced
Nov 9, 2024