Skip to content

Make proper use of the term spender... #18

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make proper use of the term spender... #18

wants to merge 2 commits into from

Conversation

mhluongo
Copy link
Member

... in ERC20WithPermit.

Closes #15

@mhluongo mhluongo requested a review from pdyraga October 25, 2024 21:10
@mhluongo mhluongo mentioned this pull request Oct 25, 2024
Open
@mhluongo mhluongo mentioned this pull request Oct 25, 2024
Open
@mhluongo
Make proper use of the term spender...
f8301fa 
... in ERC20WithPermit.

Closes #15
@mhluongo mhluongo force-pushed the erc-20-compliant-terminology branch from ab8ecd3 to f8301fa Compare October 25, 2024 21:29
@cygnusv
Copy link

cygnusv commented Oct 25, 2024

Looks good, but tests need a pass too. E.g.

solidity-contracts/test/token/ERC20WithPermit.test.js

Line 573 in c315b9d

context("when the spender has enough balance", () => {

This line uses the term "spender" to refer to the owner. Other test cases of transferOf() mention a "sender", but it's not clear if they are referring to the owner or the spender.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdyraga pdyraga Awaiting requested review from pdyraga

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

spender term in ERC20WithPermit is incorrectly used
2 participants
@mhluongo @cygnusv