Solidity best practice

[magooster]

As of solidity 0.4.17, the constant modifier for functions has been deprecated in favour of the pure or view modifiers. The contracts still contain a number of functions using the deprecated constant modifier.

Within some of the contracts I still see a mix of uint and uint256 being used (uint is an alias for uint256). IMHO best practice is to specify the size explicitly.

It might be worth fixing the version of solidity to ensure everyone is running the same tested bytecode. The current version pragma using the caret syntax allows for solidity versions >=0.4.18 <0.5.0.

In Mixer.sol the types of variables are not set explicitly (using var), lines 110,111,180.

[HarryR]

110:        var filling_id = sha256(token, denomination);
111:        var ring_guid = m_filling[filling_id];

I am happy with these being var as the compiler is making life a little easier.

The only uses of constant are:

• https://github.com/clearmatics/mobius/blob/04704b0eb1745bb0f8a57f3879c9ebfc6fdbc68c/contracts/bn256g1.sol#L56 (for constant variables, where constant is the appropriate keyword)
• https://github.com/clearmatics/mobius/blob/04704b0eb1745bb0f8a57f3879c9ebfc6fdbc68c/contracts/LinkableRing.sol#L231 - cannot use pure as the compiler complains about staticcall, view has slightly different semantics from constant, would prefer pure but can't.

Uses of uint are:

• https://github.com/clearmatics/mobius/blob/04704b0eb1745bb0f8a57f3879c9ebfc6fdbc68c/contracts/bn256g1.sol#L213 (should probably be specified as uint256)
• https://github.com/clearmatics/mobius/blob/04704b0eb1745bb0f8a57f3879c9ebfc6fdbc68c/contracts/bn256g1.sol#L229 (should probably be specified as uint256)
• https://github.com/clearmatics/mobius/blob/04704b0eb1745bb0f8a57f3879c9ebfc6fdbc68c/contracts/LinkableRing.sol#L118 (array index, use whatever fastest native size is, leave it upto compiler)
• https://github.com/clearmatics/mobius/blob/04704b0eb1745bb0f8a57f3879c9ebfc6fdbc68c/contracts/LinkableRing.sol#L133 (array index, use whatever fastest native size is, leave it upto compiler)
• https://github.com/clearmatics/mobius/blob/ad7c19b49b1774a5b78d0cbdc8492eedc0c72d0b/contracts/Mixer.sol#L224 (array index, use whatever fastest native size is, leave it upto compiler)

I'm not particularly concerned about the versions as there's no specific bugs where we need a compiler fix to work around them.

[magooster]

I thought a number of the functions in bn256g1.sol where using the constant modifier. I thought constant was reserved for state variables, and functions should use view instead (potentially can use pure if they don't read from state). constant should be an alias for view...

[HarryR]

Ah, yup I missed all the functions in bn256g1.

They should all be pure because they don't modify state.

However, solc won't allow me to use pure because I use staticcall (and I use staticcall rather than call so I can use constant or view - but they really are pure functions)

[HarryR]

Fixed in #47