Rename some ERC20 parameters to match the standard document. #3167

MicahZoltu · 2022-02-04T14:22:57Z

The variable naming was incorrect. The source of the funds is not necessarily (and in most cases isn't) the sender of the transaction. Also, this code has a msgSender which further adds confusion.

PR Checklist

Tests
Documentation
Changelog entry

The naming variable was incorrect. The source of the funds is *not* necessarily (and in most cases isn't) the sender of the transaction. Also, this code has a `msgSender` which further adds confusion.

Amxx · 2022-02-04T15:22:38Z

Hello @MicahZoltu

I'm not sure that has any impact at all, but if we should rename them then I'd suggest we use the from, to, value naming as used in the reference document.

_msgSender() is our binding into the Context pattern (for relayed meta-tx support).

MicahZoltu · 2022-02-04T16:07:15Z

Updated PR to use to/from, I agree that is a better naming scheme. I'm familiar with the _msgSender pattern the OZ contracts use, but that doesn't change the confusion introduced in this function by having two senders, one of which is not actually the "sender" of anything.

DanielVF · 2022-02-04T16:45:16Z

I love the from/to.

Looks like the mini-docs above the function need to be updated. I've sent you a PR to your PR, Micah. :D

Function documentation matches new names

Amxx · 2022-02-04T19:58:03Z

For consistency, it would also make sense to do the same changes to transfer, approve, and all the standard functions.

Also changed `msgSender` to `owner` in the approval related methods.

MicahZoltu · 2022-02-06T06:19:03Z

Updated the PR to improve consistency across methods. This includes both renaming sender/recipient to from/to and ALSO renaming _msgSender() in some places to owner (via local variable). The latter brings consistency across the approval methods and helps make the code easier to digest.

⚠️ Someone who understands _msgSender() better than I should validate that caching the result of _msgSender() in a local for the duration of these functions is not problematic! I do not know if perhaps it is intentional that _msgSender() can change within a function. If not, then I think the local is appropriate because it not only saves gas and improves readability, but it also reduces the chance that some derived class breaks something by having _msgSender() be mutable within a given call context. If it is intentional that it is mutable I will undo those changes.

Amxx · 2022-02-06T20:33:43Z

@MicahZoltu I 100% approve caching _msgSender. This function is not expected to write/change state, and in case it would return different results when called from the same context, that would be breaking.

Caching not only fixes that, but it also might save some gas!

@frangio Can you double-check before we merge?

frangio · 2022-02-07T22:54:19Z

contracts/token/ERC20/ERC20.sol

        uint256 amount
    ) public virtual override returns (bool) {
-        uint256 currentAllowance = _allowances[sender][_msgSender()];
+        address spender = _msgSender();
+        uint256 currentAllowance = allowance(from, spender);


Note: This line contains an unrelated change, the allowance is now read through an (overrideable) function instead of directly from storage. We were planning to make this change and it's small enough that I think it's ok to include here.

Yes, I included it for that reason

frangio

LGTM. Thank you @MicahZoltu for the suggestion.

I'd love to clarify one thing though. I don't believe that argument names in EIPs are normative, even in events, so this was more about improving code clarity than fixing something that was incorrect. Do we disagree on this?

DanielVF · 2022-02-08T01:05:06Z

@frangio, Yes, code clarity is the reason. The original kick off for this was a project (not using OZ) getting rekt by switching the order of their allowance lookups in one function.

The OZ code for allowances prior to this change was:

uint256 currentAllowance = _allowances[sender][_msgSender()];

Which is a bit confusing to look at (sender vs sender) and could easily lead to errors when someone was copying and modifying it.

The rest of the PR just grew from there. :)

Amxx · 2022-02-08T08:13:38Z

The name of event's arguments are important in terms of backward compatibility. Changing that could break things like the openzeppelin subgraphs

MicahZoltu · 2022-02-08T10:18:39Z

I'd love to clarify one thing though. I don't believe that argument names in EIPs are normative, even in events, so this was more about improving code clarity than fixing something that was incorrect. Do we disagree on this?

Daniel was correct, this PR was about improving clarity only. The choice of to/from wasn't even my original suggestion. 😄

* Renames `sender` to `source`. The naming variable was incorrect. The source of the funds is *not* necessarily (and in most cases isn't) the sender of the transaction. Also, this code has a `msgSender` which further adds confusion. * Changes to `from/to` instead of `source`. * Function documentation matches new names * Changed other instances of sender/recipient to from/to. Also changed `msgSender` to `owner` in the approval related methods. * apply changes to IERC20.sol + minor renaming in ERC20.sol Co-authored-by: Daniel Von Fange <daniel@leancoder.com> Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> (cherry picked from commit 63b4669)

Renames sender to source.

e14a5f2

The naming variable was incorrect. The source of the funds is *not* necessarily (and in most cases isn't) the sender of the transaction. Also, this code has a `msgSender` which further adds confusion.

Changes to from/to instead of source.

65af1e6

Function documentation matches new names

b95d737

Merge pull request #1 from DanielVF/patch-5

31e01a3

Function documentation matches new names

Changed other instances of sender/recipient to from/to.

7003838

Also changed `msgSender` to `owner` in the approval related methods.

Amxx previously approved these changes Feb 6, 2022

View reviewed changes

apply changes to IERC20.sol + minor renaming in ERC20.sol

2e35d34

Amxx dismissed their stale review via 2e35d34 February 7, 2022 15:31

Amxx approved these changes Feb 7, 2022

View reviewed changes

Amxx changed the title ~~Renames sender to source.~~ Rename some ERC20 parameters to match the standard document. Feb 7, 2022

frangio reviewed Feb 7, 2022

View reviewed changes

frangio approved these changes Feb 7, 2022

View reviewed changes

Amxx merged commit 63b4669 into OpenZeppelin:master Feb 7, 2022

This was referenced Feb 8, 2022

Add a _spendAllowance function to ERC20 & ERC777 #3170

Merged

Meta Issue: 4.5 #2900

Closed

MicahZoltu deleted the patch-4 branch February 8, 2022 10:21

frangio mentioned this pull request Feb 8, 2022

Use allowance accessor in transferFrom #3154

Closed

code423n4 mentioned this pull request Feb 23, 2022

Gas Optimizations code-423n4/2022-02-hubble-findings#102

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename some ERC20 parameters to match the standard document. #3167

Rename some ERC20 parameters to match the standard document. #3167

MicahZoltu commented Feb 4, 2022 •

edited

Loading

Amxx commented Feb 4, 2022

MicahZoltu commented Feb 4, 2022

DanielVF commented Feb 4, 2022 •

edited

Loading

Amxx commented Feb 4, 2022

MicahZoltu commented Feb 6, 2022

Amxx commented Feb 6, 2022 •

edited

Loading

frangio Feb 7, 2022

Amxx Feb 7, 2022

frangio left a comment •

edited

Loading

DanielVF commented Feb 8, 2022 •

edited

Loading

Amxx commented Feb 8, 2022

MicahZoltu commented Feb 8, 2022

Rename some ERC20 parameters to match the standard document. #3167

Rename some ERC20 parameters to match the standard document. #3167

Conversation

MicahZoltu commented Feb 4, 2022 • edited Loading

PR Checklist

Amxx commented Feb 4, 2022

MicahZoltu commented Feb 4, 2022

DanielVF commented Feb 4, 2022 • edited Loading

Amxx commented Feb 4, 2022

MicahZoltu commented Feb 6, 2022

Amxx commented Feb 6, 2022 • edited Loading

frangio Feb 7, 2022

Choose a reason for hiding this comment

Amxx Feb 7, 2022

Choose a reason for hiding this comment

frangio left a comment • edited Loading

Choose a reason for hiding this comment

DanielVF commented Feb 8, 2022 • edited Loading

Amxx commented Feb 8, 2022

MicahZoltu commented Feb 8, 2022

MicahZoltu commented Feb 4, 2022 •

edited

Loading

DanielVF commented Feb 4, 2022 •

edited

Loading

Amxx commented Feb 6, 2022 •

edited

Loading

frangio left a comment •

edited

Loading

DanielVF commented Feb 8, 2022 •

edited

Loading