Refined coding standards and cascaded changes to the code base

[AntoineRondelet]

@dtebbs - when you have a moment, please have a look at this PR.
As the name indicates, the PR fixes some inconsistencies in our contracts code, and introduces new conventions that extend the "standard set of solidity conventions" + enforces them.

The rational is: Let's stick to solidity conventions where possible, however, small amendments are made as a way to improve smart contract dev and convey more information in the object names by following conventions exposed in the CODING_STANDARDS in this PR.

The TL;DR:

• Our test contracts were named as MyContract_test.sol -> I changed to TestMyContract.sol to stick to PascalCase and remain compliant with solidity standards.
• Our test functions were named inconsistently. Some were of the form testFunction, some of the form functionTest. I switched everything to the form functionTest (arbitrary choice, could have gone the other way around - don't have strong opinion there, so happy to switch)
• Add prefix _ to all private/internal members. This was inconsistent throughout the code base
• Used camelCase for functions and their parameters for consistent interface using solidity standard
• Renamed our smart contracts using prefixes that convey the contract type (interface, library, "base contract", "test contract" etc).

Overall observation (for the sake of logging it):

• Our inheritances do not look very tight. There are a lot of inherited members that aren't used in child contracts. Not a big deal, but that may be something that could be improved in the future (surely not now) - though keeping tradeoffs between nice inheritance/abstraction layers and performances is necessary here.

Anyway, happy to discuss about the proposed conventions and keen to hear your opinion here.

[AntoineRondelet]

By refining the inheritance here it seemed that overloading verifyZkProof was useless in the past (because it was already defined in the former parent which was already concrete). Moreover, the signature of the function wasn't matching the signature of the abstract parent class (which tool a uint256 for the input instead of an array of uints..)

[AntoineRondelet]

More precisely, the snake_case convention remains broadly used in function bodies (we still have mixed styles there). The function signatures and contract's state variables/constants however should be all fine now

[AntoineRondelet]

Note to self: For #357 and to keep Zeth in standalone mode vs batch mode (with Zecale) -> we may want to go back to using an array here... Let's see when we tackle #357

[dtebbs]

Real minor details regarding the naming, so feel free to ignore. Probably not worth the effort of renaming, but just to give my thoughts:

Our test contracts were named as MyContract_test.sol -> I changed to TestMyContract.sol to stick to PascalCase and remain compliant with solidity standards.
Our test functions were named inconsistently. Some were of the form testFunction, some of the form functionTest. I switched everything to the form functionTest (arbitrary choice, could have gone the other way around - don't have strong opinion there, so happy to switch)

I'd put test at the beginning of the method names. It sounds like an action (i.e. testABC does in fact test ABC), and I've see this convention used in testing frameworks to identify test methods, so it may be more familiar to people.

Conversely, (and again a real minor detail) having test class names end with Test may be more natural (instinctively a class is more like a thing and a method could be considered like an action). Finally, having Test at the end is consistent with Base (SomethingBase for abstract classes sounds a bit more like "the base for building something on" whereas BaseSomething - which sounds a concrete "something" belonging to some "base").

• Library names must have a capital L prefix (e.g. LPairing)

Although the I prefix is very familiar, I find L to be rather unintuitive. I would probably favour either not having special names for libraries, or using SomethingLib (so class names can have a suffix Base, Lib or Test.

... or alternatively, we could consider AbstractSomething to match the keyword, and also Lib (and maybe Test) as suffices. Feels a bit unnatural for Test, but at least everything is consistent.

Again, feel free to skip this.

[dtebbs]

Our inheritances do not look very tight. There are a lot of inherited members that aren't used in child contracts. Not a big deal, but that may be something that could be improved in the future (surely not now) - though keeping tradeoffs between nice inheritance/abstraction layers and performances is necessary here.

I'm not sure I fully understand inherited members that aren't used in child contracts. Are these members that should be made private? Or members not referenced by the base class either, in which case they can be removed?

[AntoineRondelet]

Thanks @dtebbs

Real minor details regarding the naming, so feel free to ignore. Probably not worth the effort of renaming, but just to give my thoughts:

No problem, renaming isn't really painful once everything is consistent -> you know what patterns to search for - the real pain is when things aren't consistent ;)

Our test contracts were named as MyContract_test.sol -> I changed to TestMyContract.sol to stick to PascalCase and remain compliant with solidity standards.
Our test functions were named inconsistently. Some were of the form testFunction, some of the form functionTest. I switched everything to the form functionTest (arbitrary choice, could have gone the other way around - don't have strong opinion there, so happy to switch)

I'd put test at the beginning of the method names. It sounds like an action (i.e. testABC does in fact test ABC), and I've see this convention used in testing frameworks to identify test methods, so it may be more familiar to people.

Good point, let's switch to that.

Conversely, (and again a real minor detail) having test class names end with Test may be more natural (instinctively a class is more like a thing and a method could be considered like an action). Finally, having Test at the end is consistent with Base (SomethingBase for abstract classes sounds a bit more like "the base for building something on" whereas BaseSomething - which sounds a concrete "something" belonging to some "base").

FYI, the code base wasn't consistent w.r.t to the location of the "Base" in the name. In fact, if you check develop we have contracts like <name>Base and BaseMerkleTree.sol. Most of the time I find myself talking about "the base mixer" etc -> hence the name "BaseMixer" seems to better align with it (and all "Base" contracts are grouped together alphabetically which provides a better file organization IMHO).
W.r.t to the Test<name> vs <name>Test -> I'm happy to switch to <name>Test. Note that there seem to be no convention/consensus around this. If we check several projects like:

• Uniswap v3: https://github.com/Uniswap/uniswap-v3-core/tree/main/contracts/test
• Gnosis safe: https://github.com/gnosis/safe-contracts/tree/development/contracts/test
• Aragon: https://github.com/aragon/aragonOS/tree/next/contracts/test/tests

We see that we have a little bit of everything... The rationale behind using Test<name> is that (as above), the test contracts are grouped by name because they have the same prefix, and since "test" is both a name and a verb there's a bit of ambiguity here because "TestFoo" can read as "the test contract for Foo" or read as "to test sthg" (i.e. "to test Foo"). Of course, a better way to group contracts would be to introduce more structure in the contracts folder to have contracts/tests, contracts/interfaces, contacts/libraries and move away from some of the prefixes, although, prefixes convey more info in the name which comes at handy in some contexts (though we can still import as if necessary).

• Library names must have a capital L prefix (e.g. LPairing)

Although the I prefix is very familiar, I find L to be rather unintuitive. I would probably favour either not having special names for libraries, or using SomethingLib (so class names can have a suffix Base, Lib or Test.

Many libraries are named as "lib" (e.g. libfuzzer, libff, etc.) -> it seems natural to follow this prefix convention for the library names in solidity - instead of <name>Lib.sol - (definitely happy to switch the L prefix into Lib, that's a good idea!).

So, here's what we can do:

1. use Lib prefix for library files (e.g. LibPairing.sol)
2. use Base prefix for the "base" abstract classes (e.g. BaseMixer.sol) -> do you find "Abstract" to be more appropriate keyword here?
3. use the testFunction pattern for test functions, and use ContractTest for the "test" contracts -> Note that we do not have "1 test contract per contract".
4. I'll see if adding more levels to the contracts folder triggers a hell. If not, we can add subfolders to better structure the code and avoid to keep them all in the same basket (which was fine up until now, but we are starting to have quite a few contracts now)

How does that sound to you?

[AntoineRondelet]

Our inheritances do not look very tight. There are a lot of inherited members that aren't used in child contracts. Not a big deal, but that may be something that could be improved in the future (surely not now) - though keeping tradeoffs between nice inheritance/abstraction layers and performances is necessary here.

I'm not sure I fully understand inherited members that aren't used in child contracts. Are these members that should be made private? Or members not referenced by the base class either, in which case they can be removed?

Hum.. IIRC, the tool raised flags about "unused state variables" in the PGHR mixer. Such flags weren't raised for the Groth16 mixers, which seemed to imply that the inheritance structure could be refined

[dtebbs]

Our inheritances do not look very tight. There are a lot of inherited members that aren't used in child contracts. Not a big deal, but that may be something that could be improved in the future (surely not now) - though keeping tradeoffs between nice inheritance/abstraction layers and performances is necessary here.

I'm not sure I fully understand inherited members that aren't used in child contracts. Are these members that should be made private? Or members not referenced by the base class either, in which case they can be removed?

Hum.. IIRC, the tool raised flags about "unused state variables" in the PGHR mixer. Such flags weren't raised for the Groth16 mixers, which seemed to imply that the inheritance structure could be refined

Ah ... I see the point now. Thanks.
I guess there could be a few reasons for that. As you say, let's check the details in a future iteration and see if there are things that should be move.

[dtebbs]

So, here's what we can do:

1. use `Lib` prefix for library files (e.g. `LibPairing.sol`)

2. use `Base` prefix for the "base" abstract classes (e.g. `BaseMixer.sol`) -> do you find "Abstract" to be more appropriate keyword here?

3. use the `testFunction` pattern for test functions, and use `ContractTest` for the "test" contracts -> Note that we do not have "1 test contract per contract".

4. I'll see if adding more levels to the `contracts` folder triggers a hell. If not, we can add subfolders to better structure the code and avoid to keep them all in the same basket (which was fine up until now, but we are starting to have quite a few contracts now)

How does that sound to you?

Overall that sounds good.

For (2), yes, I'd be in favour of Abstract as a prefix here. I still find BaseMixer a bit unnatural and somewhat ambiguous (sounds like it could be a "class that mixes bases"), but if we switch to Abstract and Lib then to me it's al clear and consistent.

Re (3), tbh I could see an argument for either way. Given that we use prefixes elsewhere I'm happy to keep the Test... name and align with Lib, Abstract. Whichever you prefer.

For (4), moving tests to another directory, like in the other components, makes a lot of sense to me if that's not a huge problem. For other things, I'd be tempted to keep it simple for now - dividing up by type, rather than by functionality, seems like a slightly different approach than we use elsewhere. From that point of view it might be more natural to divide into mimc, merkletree, alt_bn128, groth16 etc. but that may be overkill at the moment.

[AntoineRondelet]

Overall that sounds good.

For (2), yes, I'd be in favour of Abstract as a prefix here. I still find BaseMixer a bit unnatural and somewhat ambiguous (sounds like it could be a "class that mixes bases"), but if we switch to Abstract and Lib then to me it's al clear and consistent.

Ok sounds good let's use Abstract here

Re (3), tbh I could see an argument for either way. Given that we use prefixes elsewhere I'm happy to keep the Test... name and align with Lib, Abstract. Whichever you prefer.

I have no strong preferences as per my comments above. In the absence of justification to change, let's stick to what we have now and use Test as a prefix. We can change later if this turns out to be problematic (though I doubt it)

From that point of view it might be more natural to divide into mimc, merkletree, alt_bn128, groth16 etc. but that may be overkill at the moment.

Hum... that may be overkill at the moment -> yes I agree. Such a divide looks pretty speculative as it seems to make some assumption about the future state of the code base. The current st of contract wouldn't benefit from such a granular divide IMHO. Of course, as the code base evolves, new conventions become more appropriate and new structure emerge. So, to keep things focused now, let's postpone this folder divide for later. I'll move the tests in their folder, and we'll have a handful of contracts in the same basket - which is perfectly tractable for now - especially with the newly adopted conventions on the names

[AntoineRondelet]

I'll move the tests in their folder, and we'll have a handful of contracts in the same basket - which is perfectly tractable for now - especially with the newly adopted conventions on the names

This will be one for another PR because this requires to touch the python code (beyond renaming) in order to modify the path to the contracts to deploy in mock.deploy_contract (especially in zeth.utils.get_contracts_dir())

• https://github.com/clearmatics/zeth/blob/develop/client/test_commands/mock.py#L98-L99

• zeth/client/zeth/core/utils.py

  Lines 255 to 258 in 28d901e

  	def get_contracts_dir() -> str:
  	return os.environ.get(
  	'ZETH_CONTRACTS_DIR',
  	join(get_zeth_dir(), "zeth_contracts", "contracts"))

[dtebbs]

LGTM with one comment.