search

smartcontractkit / hardhat-starter-kit

A repo for boilerplate code for testing, deploying, and shipping chainlink solidity code.
MIT License
1.21k stars 491 forks source link

Declared constants using `constants` keyword #105

Closed bobanm closed 2 years ago

bobanm commented 2 years ago

Unlike immutable variables s_subscriptionId and s_keyHash whose values are provided during contract deployment as arguments, the contract defines in advance the values for s_callbackGasLimit, s_requestConfirmations and s_numWords. Therefore, it makes more sense to declare those 3 as constant instead of immutable.

As per Solidity documentation:

Immutable variables are evaluated once at construction time and their value is copied to all the places in the code where they are accessed. For these values, 32 bytes are reserved, even if they would fit in fewer bytes. Due to this, constant values can sometimes be cheaper than immutable values.

https://docs.soliditylang.org/en/latest/contracts.html?#constant-and-immutable-state-variables

PatrickAlphaC commented 2 years ago

Thanks for this!

If we use the constant keyword, we should change the name of the variables. Like s_requestConfirmations -> REQUEST_CONFIRMATIONS

bobanm commented 2 years ago

All right, I've also changed all the constants names to UPPERCASE.

I was trying to understand which naming standard do you use? Is UPPERCASE used for all the constants and for all the contract instances, e.g. COORDINATOR?

What about the prefix s_? Is used to identify a state variable? If yes, it doesn't seem like it is used consistently, as none of the other 3 contracts in this repo use the prefix.

Do you have the coding standards documented somewhere?

PatrickAlphaC commented 2 years ago

Is UPPERCASE used for all the constants and for all the contract instances, e.g. COORDINATOR

No, just constants.

What about the prefix s_? Is used to identify a state variable? If yes, it doesn't seem like it is used consistently, as none of the other 3 contracts in this repo use the prefix.

You're right... that should be fixed!

Do you have the coding standards documented somewhere?

Most of them are pulled from the solidity style guide. I actually have had a backlogged issue to add the additional style guides somewhere... Thanks again for this PR!