[FUN-877] make persisting allowlist compatible with older contract #11907
Conversation
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
agparadiso
force-pushed
the
feature/FUN-877_persist_data_fetched_from_contracts_improv
branch
from
95c0f6f to
98744a8
Compare
| @@ -219,6 +219,17 @@ func (a *onchainAllowlist) updateFromContractV1(ctx context.Context, blockNum *b | |||
| if err != nil { | |||
| return errors.Wrap(err, "error calling GetAllAllowedSenders") | |||
| } | |||
|
|
|||
| err = a.orm.PurgeAllowedSenders() | |||
There was a problem hiding this comment.
This step to purge the DB will happen on every sync of the stored allow list right?
What is the performance impact of deleting all and re-adding? Because on the new version of the ToS contract this full purge is not needed.
Can we put the full purge behind vs. only deleting blocked users behind a feature flag? How about using the contract's typeAndVersion instead of a job spec change?
There was a problem hiding this comment.
this will only happen for the old ToS contracts that will GetAllAllowedSenders.
I like your idea of checking the contract typeAndVersion instead of a job spec change. Also the naming of the feature flag does not make sense now because its storing anyways. thanks will change that
There was a problem hiding this comment.
Now that this is backwards compatible, can the feature flag in the job spec be removed?
https://github.com/smartcontractkit/chainlink/pull/11648/files#r1462693285
agparadiso
force-pushed
the
feature/FUN-877_persist_data_fetched_from_contracts_improv
branch
from
0d35bc4 to
c8b3eda
Compare
agparadiso
force-pushed
the
feature/FUN-877_persist_data_fetched_from_contracts_improv
branch
from
c8b3eda to
7272a23
Compare
agparadiso
force-pushed
the
feature/FUN-877_persist_data_fetched_from_contracts_improv
branch
from
7272a23 to
e120b5f
Compare
| return errors.Wrap(err, "failed to extract version") | ||
| } | ||
|
|
||
| if version < strings.ReplaceAll(tosContractMinBatchProcessingVersion, ".", "") { |
There was a problem hiding this comment.
I just want to make sure that this string comparision is a safe way to determine version compatability.
Is this actually resiliant in all edge cases? Maybe we should extract this into a helper function such that we can properly unit test it? That would give me more confidence that there are no edge cases that could break this logic.
There was a problem hiding this comment.
sure I can extract it and create some unit tests for it 👍🏼
…_contracts_improv
| } | ||
|
|
||
| var errInvalidVersion = "failed to extract version: version not found in string: invalid_version" | ||
| tcs := []tc{ |
There was a problem hiding this comment.
What about these test cases?
version1: "v10.1.1",
version2: "v1.11.0",
IIUC, the current logic would say that 1011 < 1110, which is incorrect.
I wonder if there is some existing method somewhere on the internet for dealing with semantic versioning checks like this.
There was a problem hiding this comment.
There was a problem hiding this comment.
➕ thanks for spotting that, I've changed it to use semver
agparadiso
force-pushed
the
feature/FUN-877_persist_data_fetched_from_contracts_improv
branch
from
f4d90f0 to
7ca8b0d
Compare
…_contracts_improv
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
agparadiso
deleted the
feature/FUN-877_persist_data_fetched_from_contracts_improv
branch
Description
This MR complements this previous mr making the persistence of
allowlist backwards compatible with older Tos contracts.
In order to keep an updated list of allowed addresses we need to take into account the blocked addresses. Given on older contracts we don't have a method to fetch these, on each iteration, if we are fetching all the allowed addresses, we purge first all the addresses.