Closed scooprinder closed 1 year ago
Apologies for the delay @rookmate - I appreciate your feedback; are these tests still wanted? I can tidy the PR up this week if so.
Apologies for the delay @rookmate - I appreciate your feedback; are these tests still wanted? I can tidy the PR up this week if so.
Hey mate! Me and @brookr from PR #19 really want it! :pray: Although we're waiting for the repo owner to come alive and also provide feedback in the PRs...
I just noticed they did more than a few changes a few days ago :thinking: If I get some time this week I'll do another pass on the open PRs
outdated PR
I agree with the comments made in 1 and as such haven't made any modifications to errors or syntax.
Simplified the logic for the register / revoke functions, though upon further reflection these could be improved further (perhaps with different storage layouts). Knocked off around 45k gas from
register
and around 10k gas fromrevoke
.I added some simple forge tests to cover the base use cases and found the following issues:
require(registeredDelegation[globalHash] == false);
check inregisterDelegationAddress
means that it is impossible to add more than onedelegationAddresses
per hash (which makes the most of the mapping redundant).retrieveActiveFromDelegations
andretrieveActiveToDelegations
create fixed size arrays matching all delegations. This means that it could return an array with address(0) elements. For example, if thedelegationAddresses[]
for a given hash has 3 entries, two of which are expired in slots 0 and 1 then with the current implementation would return[0x000..., 0x000..., <delegation address>]
- for large array sizes this would require further processing to get the correct information.length
of the returningdelegationAddresses[]
for a given hash.The code in this PR isn't perfect either (only had a little bit of time) but I think some more fundamental changes are required - I'm submitting it because it could be some help but no drama if it isn't.