search

cartesi / rollups-contracts

Smart Contracts for Cartesi Rollups
https://cartesi.github.io/rollups-contracts/
Apache License 2.0
17 stars 37 forks source link

refactor!: type input indices as `uint64` #210

Closed guidanoli closed 6 months ago

guidanoli commented 6 months ago

Close #209

ZzzzHui commented 6 months ago

Even if we want to save space for events, we don't need to use uint64 in every intermediary step, for example, local variables and function arguments. Because in those intermediary steps, if we cast them as uint64, they would be auto cast back to uint256 for processing. So we should still use uint256 as much as possible. Plus, events doesn't cost too much gas, ref: https://github.com/wolflo/evm-opcodes/blob/main/gas.md#a8-log-operations So if we run gas report on this PR, we can see gas costs go up.

guidanoli commented 6 months ago

@ZzzzHui There is no reason to use uint256 if we only need uint64. It might even confuse users of the contracts thinking that these values might ever go above $2^{64}$. As for internal variables, if the gas changes are not that dramatic, I'd also keep them as uint64 for readability and consistency.

guidanoli commented 6 months ago

Here is the difference in gas costs from main to feature/input-idx-uint64. Cost-wise, I don't think it's too relevant of a cost change. The greatest changes, in absolute terms, are on deployments, which would cost ~$2 more.

Contract Function Min Avg Max
Application 39640 (3.06%)
Application executeVoucher 804 (12.41%) 877 (1.63%) 927 (0.94%)
Application getInputRelays -22 (-0.07%) -22 (-0.06%) -22 (-0.04%)
Application validateNotice 878 (2.83%) 878 (2.55%) 878 (2.31%)
Application wasVoucherExecuted 253 (41.00%) 253 (19.72%) 253 (9.67%)
ApplicationFactory 39642 (2.55%)
ApplicationFactory calculateApplicationAddress 573 (0.67%) 573 (0.67%) 573 (0.67%)
ApplicationFactory newApplication(address,address,address[],address,bytes32) 39664 (1.07%) 39664 (1.07%) 39664 (1.07%)
ApplicationFactory newApplication(address,address,address[],address,bytes32,bytes32) 39703 (0.74%) 13282 (0.00%) -39561 (-0.00%)
ApplicationTest 127441 (1.27%)
ApplicationTest calculateInputIndex 221 (28.01%) 221 (28.01%) 221 (28.01%)
Authority 48449 (22.77%)
Authority getEpochHash 449 (60.19%) 449 (60.19%) 449 (60.19%)
Authority owner -33 (-9.57%) -33 (-9.57%) -33 (-9.57%)
Authority submitClaim 505 (4.54%) 757 (2.77%)
AuthorityFactory 48450 (11.83%)
AuthorityFactory calculateAuthorityAddress 621 (13.57%) 621 (13.57%) 621 (13.57%)
AuthorityFactory newAuthority(address) 48492 (19.64%) 48492 (19.64%) 48492 (19.64%)
AuthorityFactory newAuthority(address,bytes32) 48534 (19.64%) 167 (0.00%) -48199 (-0.00%)
Quorum 63262 (21.01%)
Quorum numOfValidators 45 (24.86%) 45 (24.86%) 45 (24.86%)
Quorum validatorById -22 (-4.21%) -74 (-12.89%) -2022 (-80.17%)
Quorum validatorId -95 (-7.96%)
QuorumFactory 63258 (10.17%)
QuorumFactory calculateQuorumAddress 892 (11.75%) 892 (11.75%) 892 (11.75%)
QuorumFactory newQuorum(address[]) 63294 (3.97%) 63294 (3.97%) 63294 (3.97%)
QuorumFactory newQuorum(address[],bytes32) 63385 (16.65%) 108 (0.00%) -63169 (-0.00%)
SimpleConsensus 24424 (26.05%)
ZzzzHui commented 6 months ago

It's a common practice to use uint256 even for numbers that will never go that large. For example, in the OpenZeppelin's BitMaps library, the variable uint256 bucket will never go above uint248, thus it's more readable using uint248, but uint256 is used for optimization reasons. The greatest gas cost change is the deployments, but the cost change for functions occurs every time the function executes. Although it's not much cost change for just one execution, I'm not sure whether it's necessary for this change.