Fix/rename anyone account #1357

[ckshei]

Fixes #1357 - #1357

Changing all references of an "anyone" account to "other" but keeping descriptions intact.

The "anyone" account is not the best name for a random account with no special permissions. "Other" was decided to be a better descriptor.

[nventuro]

Great work @ckshei, thanks a lot!

You mentioned you found an unused instance of other, where is that? The linter should've caught such a thing, so it's probably a false positive.

[ckshei]

@nventuro Thank you so much! I'm trying to get more involved with OS contributions, and this was one of my firsts commits to an OS project. Your responsiveness + direction made it super easy :)

The example I saw was in ERC721MintBurn.behavior.js. It's passed in the array. I'm still trying to wrap my head around where these accounts are coming from

function shouldBehaveLikeMintAndBurnERC721 (
  creator,
  minter,
  [owner, newOwner, approved, other]
) {

[nventuro]

Huh, it looks like you're right! I wonder why the linter isn't detecting that as an unused variable. Oh well. Mind opening a PR to remove it? :)

That helper function is called in ERC721Mintable.test.js:

contract('ERC721Mintable', function ([_, creator, ...accounts]) {
  ...
  shouldBehaveLikeMintAndBurnERC721(creator, minter, accounts);
});

so the array of received accounts are simply the accounts that were not used in the main test (i.e. the ones in ...accounts).

[ckshei]

I saw that in ERC721Mintable.test.js, but I wasn't sure where the ...accounts are being passed in from. I guess I'm not so clear as to who calls these test functions and passes them their params.

Another couple unused variables are owner and operator in the file ERC721Pausable.test.js

contract('ERC721Pausable', function ([
  _,
  creator,
  owner,
  operator,
  otherPauser,
  ...accounts
]) {

Should I set those as _?
I can definitely open a PR to take care of this! After that, what do you think would be a good next issue to work on?

[nventuro]

I saw that in ERC721Mintable.test.js, but I wasn't sure where the ...accounts are being passed in from. I guess I'm not so clear as to who calls these test functions and passes them their params.

Ah, that'd be truffle. It uses mocha under the hood, and passes a set of accounts when you use the contract block.

Another couple unused variables are owner and operator in the file ERC721Pausable.test.js

Those look like the result of a nasty copy-paste 😅 Please do!

After that, what do you think would be a good next issue to work on?

Since you're already working on the test suite, maybe you could tackle #1687? It's a bit more involved than this one, but it could help you get more familiar with the test suite code base, and with chai (the last piece of the puzzle in the testing setup we use).

[ckshei]

Ah, that'd be truffle. It uses mocha under the hood, and passes a set of accounts when you use the contract block.

Got it - so mocha passes a set of accounts as an array when you use the contract block, and we're just labeling specific accounts different names? That makes sense.

So should I remove the _ in ERC721Pausable.test.js as well in my PR which removes the declared but unused variables?

contract('ERC721Pausable', function ([
  _,
  creator,
  otherPauser,
  ...accounts

[nventuro]

and we're just labeling specific accounts different names?

Correct!

So should I remove the _ in ERC721Pausable.test.js

No, the reason why we have _ is that the first account in the array is the account truffle uses for transactions for which the from is not specified (e.g. when you do token.transfer(recipient, amount) instead of token.transfer(recipient, amount, { from: sender })). If we did use that first account, it could happen that there was some special behaviour associated with it, e.g. it might be the admin of a contract (if you do [creator, otherPauser, other]), and tests which don't specify the from end up being called from the admin, which may both cause unexpected issues, but also to make you think you're testing stuff you really aren't. So we simply label it _ in order for no test to use it explicitly (similar to when you do { from: other }).

Does that make sense? We've been wanting to write a testing guide (#1077) for some time explaining these sort of things, but never got to it.

[ckshei]

Yes, I think it makes sense. Basically, Truffle defaults to using the first account in its array for any actions in which you don't specify a sender.

If we also happen to set that first account to be the creator account, and then we write a test where we don't specify a from: other, the test would run with the creator account which could result in unintended consequences.

By ensuring the first account has no special privileges, we're adding a bit of redundancy in our tests, so that if an account isn't specified for a test, the test defaults to an account with no special permissions. Is that the gist of it?

[nventuro]

That is absolutely right, yes, and you worded it much better than I did :)