Enumberable set cleanup #12885
Enumberable set cleanup #12885
Conversation
|
I see you updated files related to
|
|
I see you updated files related to |
shileiwill
force-pushed
the
enumberableSet
branch
from
e766fd6
to
9ed54b9
Compare
shileiwill
force-pushed
the
enumberableSet
branch
from
9ed54b9
to
1c111e2
Compare
| for (uint256 idx = 0; idx < s_registrars.length(); idx++) { | ||
| s_registrars.remove(s_registrars.at(idx)); | ||
| } |
There was a problem hiding this comment.
I think this is potentially buggy because remove() will adjust the index of items in the set on each delete. I think we need to count backwards instead, like this:
for (int256 idx = int256(s_registrars.length()); idx > -1; idx--) {
s_registrars.remove(s_registrars.at(idx));
}There was a problem hiding this comment.
wow, good catch!
I just tested in remix, and indeed, using the above code couldnt remove all, it will have leftover. The right way is to cleanup using reverse order, as you recommended.
I added a foundry test to cover this scenario. If you see this commit , it fails bc of registrar length doesnt match.
with the latest commit, s_registrars is cleaned up properly.
| for (uint256 idx = 0; idx < s_deactivatedTransmitters.length(); idx++) { | ||
| s_deactivatedTransmitters.remove(s_deactivatedTransmitters.at(idx)); | ||
| } |
There was a problem hiding this comment.
since s_deactivatedTransmitters is not exposed, i couldnt unit test this guy. I hope the above registrar test can also verify the logic.
shileiwill
force-pushed
the
enumberableSet
branch
from
1c111e2
to
af2190b
Compare
shileiwill
force-pushed
the
enumberableSet
branch
from
af2190b
to
03a7880
Compare
| ( | ||
| , | ||
| IAutomationV21PlusCommon.OnchainConfigLegacy memory onchainConfig2, | ||
| address[] memory signers, | ||
| address[] memory transmitters, | ||
| uint8 f | ||
| ) = registry.getState(); | ||
|
|
||
| assertEq(onchainConfig2.registrars.length, 3); |
There was a problem hiding this comment.
Can we test the values of the new registrars, instead of just the length? Also, I think this bug is only exposed when decreasing registrars. Could we extend this test so that we decrease from say 3 to 0 and test that all have been removed?
There was a problem hiding this comment.
Can we test the values of the new registrars, instead of just the length?
sure, will do.
I think this bug is only exposed when decreasing registrars. Could we extend this test so that we decrease from say 3 to 0 and test that all have been removed?
This should be already covered. When we setConfig, we remove all existing registrars and set new ones. If you check out this particular commit, and run the foundry test, you should see the test failure. I left it there in a separate commit to showcase this.
There was a problem hiding this comment.
awesome - sorry I missed that
| function _getRegistrars() private pure returns (address[] memory) { | ||
| address[] memory registrars = new address[](2); | ||
| registrars[0] = address(uint160(uint256(keccak256("registrar1")))); | ||
| registrars[1] = address(uint160(uint256(keccak256("registrar2")))); | ||
| return registrars; | ||
| } |
There was a problem hiding this comment.
nitpick - but since the function is only used once, it might be neater to just inline it? not a strong opinion
There was a problem hiding this comment.
The reason of having this function is that we cant initiate and add elements to the registrars array outside of a function. If you see the original code, the registrars were under contracts.
shileiwill
force-pushed
the
enumberableSet
branch
from
03a7880
to
af3b5db
Compare
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
No description provided.