Nonfunctional typos #1643 #1652
Conversation
The Ownable contract has a comment explaining that renouncing ownership will prevent execution of functions with the onlyOwner modifier. This commit moves that comment to the @dev section and replaces it with a description suitable for a generic user.
FinalizableCrowdsale and RefundableCrowsale both have comments indicating that they are extensions of the Crowdsale contract. This commit refines those comments to the most immediate ancestor ( TimedCrowdsale and RefundableCrowdsale respectively )
The SignatureBouncer contract has modifiers to validate the message sender is authorised to perform an action. They pass msg.sender to internal functions as the variable `account`, but the function comments refer to the variable as `sender` This commit changes the variable name to `sender`
nikeshnazareth
force-pushed
the
nonfunctional-typos-#1643
branch
from
e090ba1 to
608beeb
Compare
| @@ -75,36 +75,36 @@ contract SignatureBouncer is SignerRole { | |||
| * @dev is the signature of `this + sender` from a signer? | |||
| * @return bool | |||
| */ | |||
| function _isValidSignature(address account, bytes memory signature) internal view returns (bool) { | |||
| return _isValidDataHash(keccak256(abi.encodePacked(address(this), account)), signature); | |||
| function _isValidSignature(address sender, bytes memory signature) internal view returns (bool) { | |||
There was a problem hiding this comment.
I'd go the other way around with this one: the signer isn't necessarily the sender of the message, so I think the more generic account fits better. What we should do is update the comments so that they no longer read 'sender'.
There was a problem hiding this comment.
No problem - I'm really excited to start contributing to the ecosystem! Thanks for being so welcoming 😀
With respect to the SignatureBouncer, if I've understood the contract correctly, the signer isn't necessarily the sender but the account variable is (at least in the base contract). The onlyValidSignature[And..] modifiers are ensuring that the current message sender has been authorised to call the function by some unspecified signer.
On the other hand, I could imagine derived contracts that use the _isValidSignature[And..] set of functions to ensure that a signer has authorised a third party to perform an action on the contract, so your instinct to avoid tightly coupling the function to its current use makes a lot of sense. Happy to follow your lead.
I've updated the PR accordingly 😄
There was a problem hiding this comment.
Yes, that is precisely the reason why - those functions could even be abstracted away to some sort of signature utilities contract that SignatureBouncer uses.
|
Regarding commits, don't worry about it - we always squash-merge. Keeping a clean commit history does make it easier to review in some scenarios though :) |
The SignatureBouncer has comments that use the description `sender` to refer to the variable `account`. This commit updates the comments for consistency. Maintainer Note: this reverts changes in the previous commit, which renamed the variable `account` instead.
Fixes #1643
This PR
There are no new tests.
I wasn't sure if you usually squash commits before or after reviewing the PR. I'm happy to squash them if you prefer.