Use uint256 iterators in MerkleTree loops

[heueristik]

🧐 Motivation

The current MerkleTree.sol contract contains two for loops, both of which use uint32 as an iterator when iterating from 0 to uint8 treeDepth. This is not optimal and uint256 should be used to minimize conversions.

📝 Details

Use uint256 for the iterators in MerkleTree.sol

openzeppelin-contracts/contracts/utils/structs/MerkleTree.sol

Line 91 in bf69b60

for (uint32 i = 0; i < treeDepth; ++i) {

openzeppelin-contracts/contracts/utils/structs/MerkleTree.sol

Line 146 in bf69b60

for (uint32 i = 0; i < treeDepth; i++) {

to save gas.

While on it, you can also change the for loops in the Heap.t.sol test.

openzeppelin-contracts/test/utils/structs/Heap.t.sol

Line 16 in bf69b60

for (uint32 i = 1; i < heap.length(); ++i) {

[heueristik]

Let me know if I should add the change in Heap.t.sol to #5415.

[ernestognw]

Hi @heueristik, thanks for sharing.

There are some gas savings during construction (I wasn't expecting it) but not much during runtime.

Although the savings seem considerable, applying this change is considered a breaking change given the arguments will be encoded differently after merging. I'm not 100% convinced the change is worth changing the interface

[heueristik]

applying this change is considered a breaking change given the arguments will be encoded differently after merging

Thank you @ernestognw for taking a look. Can you help me to understand which arguments will be encoded differently?
The loop counters i are only passed to

openzeppelin-contracts/contracts/utils/Arrays.sol

Line 383 in bf69b60

function unsafeAccess(address[] storage arr, uint256 pos) internal pure returns (StorageSlot.AddressSlot storage) {

for uint256 pos. What will change and break because of this?

[ernestognw]

Hey @heueristik, sorry my bad, I thought these were arguments to a function. That is not the case.

Have you done some benchmarking? I suspect there may be no gas difference when using --via-ir

[arr00]

I'm under the impression that there are no breaking changes here. I think we should merge it since using uint256 iterators is best practice.

[ernestognw]

Let me know if I should add the change in Heap.t.sol to #5415.

Yeah I think this make sense too, feel free to send a PR. It's just a change in tests so I don't think it's a blocker. I moved on and merged #5415