[FUN-1332] Allowlist optimisation #12588
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I see you updated files related to
|
agparadiso
changed the title
KuphJr
reviewed
Mar 26, 2024
agparadiso
force-pushed
the
feat/FUN-1332_allowlist_optimization
branch
from
42994fc to
7a96c97
Compare
agparadiso
force-pushed
the
feat/FUN-1332_allowlist_optimization
branch
from
7a96c97 to
a234f13
Compare
agparadiso
force-pushed
the
feat/FUN-1332_allowlist_optimization
branch
from
5f1edc5 to
c2cbd78
Compare
agparadiso
force-pushed
the
feat/FUN-1332_allowlist_optimization
branch
from
fda542a to
7170ee0
Compare
agparadiso
force-pushed
the
feat/FUN-1332_allowlist_optimization
branch
from
f440ada to
d2d6bd1
Compare
agparadiso
force-pushed
the
feat/FUN-1332_allowlist_optimization
branch
from
d2d6bd1 to
476651c
Compare
bolekk
reviewed
May 16, 2024
| idxEnd := idxStart + uint64(a.config.OnchainAllowlistBatchSize) | ||
| if idxEnd >= count { | ||
| idxEnd = count - 1 | ||
| idxStart := uint64(i) - uint64(a.config.OnchainAllowlistBatchSize) |
There was a problem hiding this comment.
What if count is less than batch size?
There was a problem hiding this comment.
good catch, I was expecting uint to handle this if the subtraction was less than 0. TIL 🙇🏼
| idxEnd = count - 1 | ||
| idxStart := uint64(i) - uint64(a.config.OnchainAllowlistBatchSize) | ||
| idxEnd := uint64(i) - 1 | ||
| if idxEnd >= currentAllowedSenderCount { |
There was a problem hiding this comment.
no, its a leftover, thanks!
| } | ||
|
|
||
| if updatedAllowedSenderCount > currentAllowedSenderCount { | ||
| lastBatchIdxStart := updatedAllowedSenderCount - (updatedAllowedSenderCount - currentAllowedSenderCount) |
There was a problem hiding this comment.
this is equal to currentAllowedSenderCount
There was a problem hiding this comment.
true! I placed it as I thought it, but indeed:
b - (b - a) = a
| } | ||
|
|
||
| // allowlistSize defines how big the mocked allowlist will be | ||
| allowlistSize := 50 |
There was a problem hiding this comment.
Can you add a test case where this is smaller than batch size please?
agparadiso
force-pushed
the
feat/FUN-1332_allowlist_optimization
branch
from
e1246fe to
528d283
Compare
bolekk
approved these changes
May 20, 2024
|
KuphJr
approved these changes
May 23, 2024
cedric-cordenier
pushed a commit
that referenced
this pull request
May 29, 2024
* feat: update allowlist in batches giving priority to latest allowed addresses * fix: adjust iteration and add tests on updateAllowedSendersInBatches * fix: make a deep copy of the map to avoid race conditions * feat: extra step to fetch latest added addresses while batching * fix: check allowlist size is bigger than the batchsize * chore: remove leftover and add modify tests to be closer to a real scenario * chore: simplify lastBatchIdxStart * chore: remove newlines to pass sonarqube check
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
this pr introduces an optimization for allowlist by updating it in batches giving priority to latest allowed addresses and adding an extra step on the batching process in which we check for the length of the allowlist and if it changed (new addresses were added) we fetch these before continuing the loop
What changed
We are adding an extra step that will fetch the latest addresses added IF the length of the allowlist was increased.
We were already fetching the allowlist from the contract in batches, but we were updating the node representation only when the full list was fetched. This represented a delay for users when a new address was added, which would grow proportional to the increase of the allowlist length.
In order to mitigate this, we are updating the nodes representation with every batch we fetch.
Because the nodes representation uses a
atomic.Pointer[map[common.Address]struct{}]we only have aStoremethod to replace completely the current list.So we what we are doing is:
a.allowlist.Load()by doing this the user should be able to see the new added addresses faster than waiting until the full list is fetched.
miro diagram here
How is this tested
This is tested with white box unit tests that mock the onchain part and allows to validate the correct fetching order