From fdcd5fa5b411c11ed1e03f94520d02c5ef141b2f Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Thu, 30 Aug 2018 02:41:39 +0000 Subject: [PATCH 1/2] Remove underscores from event parameters. Fixes #1175 --- CODE_STYLE.md | 3 + contracts/mocks/ERC721ReceiverMock.sol | 10 ++-- contracts/token/ERC721/ERC721Basic.sol | 18 +++--- .../token/ERC721/ERC721BasicToken.behavior.js | 58 +++++++++---------- test/token/ERC721/ERC721MintBurn.behavior.js | 12 ++-- 5 files changed, 52 insertions(+), 49 deletions(-) diff --git a/CODE_STYLE.md b/CODE_STYLE.md index 593f35cb23d..22bdf77478e 100644 --- a/CODE_STYLE.md +++ b/CODE_STYLE.md @@ -24,6 +24,9 @@ Any exception or additions specific to our project are documented below. } ``` + The exception are the parameters of events. There is not chance of ambiguity + with these, so they should not have underscores. + * Internal and private state variables should have an underscore suffix. ``` diff --git a/contracts/mocks/ERC721ReceiverMock.sol b/contracts/mocks/ERC721ReceiverMock.sol index 82b8a2610dc..a6c3bed1da4 100644 --- a/contracts/mocks/ERC721ReceiverMock.sol +++ b/contracts/mocks/ERC721ReceiverMock.sol @@ -8,11 +8,11 @@ contract ERC721ReceiverMock is ERC721Receiver { bool reverts_; event Received( - address _operator, - address _from, - uint256 _tokenId, - bytes _data, - uint256 _gas + address operator, + address from, + uint256 tokenId, + bytes data, + uint256 gas ); constructor(bytes4 _retval, bool _reverts) public { diff --git a/contracts/token/ERC721/ERC721Basic.sol b/contracts/token/ERC721/ERC721Basic.sol index f9075f06004..1c7ca9a0eb6 100644 --- a/contracts/token/ERC721/ERC721Basic.sol +++ b/contracts/token/ERC721/ERC721Basic.sol @@ -40,19 +40,19 @@ contract ERC721Basic is ERC165 { */ event Transfer( - address indexed _from, - address indexed _to, - uint256 indexed _tokenId + address indexed from, + address indexed to, + uint256 indexed tokenId ); event Approval( - address indexed _owner, - address indexed _approved, - uint256 indexed _tokenId + address indexed owner, + address indexed approved, + uint256 indexed tokenId ); event ApprovalForAll( - address indexed _owner, - address indexed _operator, - bool _approved + address indexed owner, + address indexed operator, + bool approved ); function balanceOf(address _owner) public view returns (uint256 _balance); diff --git a/test/token/ERC721/ERC721BasicToken.behavior.js b/test/token/ERC721/ERC721BasicToken.behavior.js index 3eec9ff92f0..3ecd5e41f78 100644 --- a/test/token/ERC721/ERC721BasicToken.behavior.js +++ b/test/token/ERC721/ERC721BasicToken.behavior.js @@ -92,17 +92,17 @@ function shouldBehaveLikeERC721BasicToken (accounts) { it('emit only a transfer event', async function () { logs.length.should.be.equal(1); logs[0].event.should.be.equal('Transfer'); - logs[0].args._from.should.be.equal(owner); - logs[0].args._to.should.be.equal(this.to); - logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + logs[0].args.from.should.be.equal(owner); + logs[0].args.to.should.be.equal(this.to); + logs[0].args.tokenId.should.be.bignumber.equal(tokenId); }); } else { it('emits only a transfer event', async function () { logs.length.should.be.equal(1); logs[0].event.should.be.equal('Transfer'); - logs[0].args._from.should.be.equal(owner); - logs[0].args._to.should.be.equal(this.to); - logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + logs[0].args.from.should.be.equal(owner); + logs[0].args.to.should.be.equal(this.to); + logs[0].args.tokenId.should.be.bignumber.equal(tokenId); }); } @@ -167,9 +167,9 @@ function shouldBehaveLikeERC721BasicToken (accounts) { it('emits only a transfer event', async function () { logs.length.should.be.equal(1); logs[0].event.should.be.equal('Transfer'); - logs[0].args._from.should.be.equal(owner); - logs[0].args._to.should.be.equal(owner); - logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + logs[0].args.from.should.be.equal(owner); + logs[0].args.to.should.be.equal(owner); + logs[0].args.tokenId.should.be.bignumber.equal(tokenId); }); it('keeps the owner balance', async function () { @@ -247,10 +247,10 @@ function shouldBehaveLikeERC721BasicToken (accounts) { result.receipt.logs.length.should.be.equal(2); const [log] = decodeLogs([result.receipt.logs[1]], ERC721Receiver, this.receiver.address); log.event.should.be.equal('Received'); - log.args._operator.should.be.equal(owner); - log.args._from.should.be.equal(owner); - log.args._tokenId.toNumber().should.be.equal(tokenId); - log.args._data.should.be.equal(data); + log.args.operator.should.be.equal(owner); + log.args.from.should.be.equal(owner); + log.args.tokenId.toNumber().should.be.equal(tokenId); + log.args.data.should.be.equal(data); }); it('should call onERC721Received from approved', async function () { @@ -258,10 +258,10 @@ function shouldBehaveLikeERC721BasicToken (accounts) { result.receipt.logs.length.should.be.equal(2); const [log] = decodeLogs([result.receipt.logs[1]], ERC721Receiver, this.receiver.address); log.event.should.be.equal('Received'); - log.args._operator.should.be.equal(approved); - log.args._from.should.be.equal(owner); - log.args._tokenId.toNumber().should.be.equal(tokenId); - log.args._data.should.be.equal(data); + log.args.operator.should.be.equal(approved); + log.args.from.should.be.equal(owner); + log.args.tokenId.toNumber().should.be.equal(tokenId); + log.args.data.should.be.equal(data); }); describe('with an invalid token id', function () { @@ -334,9 +334,9 @@ function shouldBehaveLikeERC721BasicToken (accounts) { it('emits an approval event', async function () { logs.length.should.be.equal(1); logs[0].event.should.be.equal('Approval'); - logs[0].args._owner.should.be.equal(sender); - logs[0].args._approved.should.be.equal(address); - logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + logs[0].args.owner.should.be.equal(sender); + logs[0].args.approved.should.be.equal(address); + logs[0].args.tokenId.should.be.bignumber.equal(tokenId); }); }; @@ -447,9 +447,9 @@ function shouldBehaveLikeERC721BasicToken (accounts) { logs.length.should.be.equal(1); logs[0].event.should.be.equal('ApprovalForAll'); - logs[0].args._owner.should.be.equal(sender); - logs[0].args._operator.should.be.equal(operator); - logs[0].args._approved.should.equal(true); + logs[0].args.owner.should.be.equal(sender); + logs[0].args.operator.should.be.equal(operator); + logs[0].args.approved.should.equal(true); }); }); @@ -469,9 +469,9 @@ function shouldBehaveLikeERC721BasicToken (accounts) { logs.length.should.be.equal(1); logs[0].event.should.be.equal('ApprovalForAll'); - logs[0].args._owner.should.be.equal(sender); - logs[0].args._operator.should.be.equal(operator); - logs[0].args._approved.should.equal(true); + logs[0].args.owner.should.be.equal(sender); + logs[0].args.operator.should.be.equal(operator); + logs[0].args.approved.should.equal(true); }); it('can unset the operator approval', async function () { @@ -497,9 +497,9 @@ function shouldBehaveLikeERC721BasicToken (accounts) { logs.length.should.be.equal(1); logs[0].event.should.be.equal('ApprovalForAll'); - logs[0].args._owner.should.be.equal(sender); - logs[0].args._operator.should.be.equal(operator); - logs[0].args._approved.should.equal(true); + logs[0].args.owner.should.be.equal(sender); + logs[0].args.operator.should.be.equal(operator); + logs[0].args.approved.should.equal(true); }); }); }); diff --git a/test/token/ERC721/ERC721MintBurn.behavior.js b/test/token/ERC721/ERC721MintBurn.behavior.js index c09480a38b7..1e7135b4563 100644 --- a/test/token/ERC721/ERC721MintBurn.behavior.js +++ b/test/token/ERC721/ERC721MintBurn.behavior.js @@ -40,9 +40,9 @@ function shouldBehaveLikeMintAndBurnERC721Token (accounts) { it('emits a transfer event', async function () { logs.length.should.be.equal(1); logs[0].event.should.be.equal('Transfer'); - logs[0].args._from.should.be.equal(ZERO_ADDRESS); - logs[0].args._to.should.be.equal(to); - logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + logs[0].args.from.should.be.equal(ZERO_ADDRESS); + logs[0].args.to.should.be.equal(to); + logs[0].args.tokenId.should.be.bignumber.equal(tokenId); }); }); @@ -78,9 +78,9 @@ function shouldBehaveLikeMintAndBurnERC721Token (accounts) { it('emits a burn event', async function () { logs.length.should.be.equal(1); logs[0].event.should.be.equal('Transfer'); - logs[0].args._from.should.be.equal(sender); - logs[0].args._to.should.be.equal(ZERO_ADDRESS); - logs[0].args._tokenId.should.be.bignumber.equal(tokenId); + logs[0].args.from.should.be.equal(sender); + logs[0].args.to.should.be.equal(ZERO_ADDRESS); + logs[0].args.tokenId.should.be.bignumber.equal(tokenId); }); }); From eed70290dcf2088e69f9942baa8284987ed9cc3c Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 03:01:35 +0000 Subject: [PATCH 2/2] Add comment about ERC --- CODE_STYLE.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/CODE_STYLE.md b/CODE_STYLE.md index 22bdf77478e..3eca35f86ac 100644 --- a/CODE_STYLE.md +++ b/CODE_STYLE.md @@ -24,8 +24,11 @@ Any exception or additions specific to our project are documented below. } ``` - The exception are the parameters of events. There is not chance of ambiguity - with these, so they should not have underscores. + The exception are the parameters of events. There is no chance of ambiguity + with these, so they should not have underscores. Not even if they are + specified on an ERC with underscores; removing them doesn't change the ABI, + so we should be consistent with the rest of the events in this repository + and remove them. * Internal and private state variables should have an underscore suffix.