-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FUN-877] make persisting allowlist compatible with older contract #11907
Merged
agparadiso
merged 6 commits into
develop
from
feature/FUN-877_persist_data_fetched_from_contracts_improv
Feb 22, 2024
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
b28c4b3
feat: make persisting allowlist compatible with older contract
agparadiso e120b5f
feat: check tos contract version instead of a config feature flag
agparadiso 0da1ede
extract contract version comparison to helper function
agparadiso 7514edd
Merge branch 'develop' into feature/FUN-877_persist_data_fetched_from…
agparadiso 7ca8b0d
fix: use semver to compare versions
agparadiso 8771f86
Merge branch 'develop' into feature/FUN-877_persist_data_fetched_from…
agparadiso File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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