Skip to content
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.

Sign up for GitHub

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
merged 6 commits into from
Feb 22, 2024
Merged

[FUN-877] make persisting allowlist compatible with older contract #11907

Show file tree
Hide file tree
Changes from 2 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 Jan 26, 2024
e120b5f
feat: check tos contract version instead of a config feature flag
agparadiso Feb 16, 2024
0da1ede
extract contract version comparison to helper function
agparadiso Feb 21, 2024
7514edd
Merge branch 'develop' into feature/FUN-877_persist_data_fetched_from…
agparadiso Feb 21, 2024
7ca8b0d
fix: use semver to compare versions
agparadiso Feb 22, 2024
8771f86
Merge branch 'develop' into feature/FUN-877_persist_data_fetched_from…
agparadiso Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 43 additions & 6 deletions core/services/gateway/handlers/functions/allowlist/allowlist.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"encoding/hex"
"fmt"
"math/big"
"regexp"
"strings"
"sync"
"sync/atomic"
"time"
Expand All @@ -23,9 +25,10 @@ import (
)

const (
defaultStoredAllowlistBatchSize = 1000
defaultOnchainAllowlistBatchSize = 100
defaultFetchingDelayInRangeSec = 1
defaultStoredAllowlistBatchSize = 1000
defaultOnchainAllowlistBatchSize = 100
defaultFetchingDelayInRangeSec = 1
tosContractMinBatchProcessingVersion = "1.1.0"
)

type OnchainAllowlistConfig struct {
Expand All @@ -38,8 +41,6 @@ type OnchainAllowlistConfig struct {
UpdateTimeoutSec uint `json:"updateTimeoutSec"`
StoredAllowlistBatchSize uint `json:"storedAllowlistBatchSize"`
OnchainAllowlistBatchSize uint `json:"onchainAllowlistBatchSize"`
// StoreAllowedSendersEnabled is a feature flag that enables storing in db a copy of the allowlist.
StoreAllowedSendersEnabled bool `json:"storeAllowedSendersEnabled"`
// FetchingDelayInRangeSec prevents RPC client being rate limited when fetching the allowlist in ranges.
FetchingDelayInRangeSec uint `json:"fetchingDelayInRangeSec"`
}
Expand Down Expand Up @@ -210,7 +211,21 @@ func (a *onchainAllowlist) updateFromContractV1(ctx context.Context, blockNum *b
}

var allowedSenderList []common.Address
if !a.config.StoreAllowedSendersEnabled {
typeAndVersion, err := tosContract.TypeAndVersion(&bind.CallOpts{
Pending: false,
BlockNumber: blockNum,
Context: ctx,
})
if err != nil {
return errors.Wrap(err, "failed to fetch the tos contract type and version")
}

version, err := extractContractVersion(typeAndVersion)
if err != nil {
return errors.Wrap(err, "failed to extract version")
}

if version < strings.ReplaceAll(tosContractMinBatchProcessingVersion, ".", "") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure I can extract it and create some unit tests for it 👍🏼

allowedSenderList, err = tosContract.GetAllAllowedSenders(&bind.CallOpts{
Pending: false,
BlockNumber: blockNum,
Expand All @@ -219,6 +234,17 @@ func (a *onchainAllowlist) updateFromContractV1(ctx context.Context, blockNum *b
if err != nil {
return errors.Wrap(err, "error calling GetAllAllowedSenders")
}

err = a.orm.PurgeAllowedSenders()
Copy link
Contributor

@justinkaseman justinkaseman Feb 2, 2024 •
edited
Loading

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?

Copy link
Contributor Author

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

if err != nil {
a.lggr.Errorf("failed to purge allowedSenderList: %w", err)
}

err = a.orm.CreateAllowedSenders(allowedSenderList)
if err != nil {
a.lggr.Errorf("failed to update stored allowedSenderList: %w", err)
}

} else {
err = a.syncBlockedSenders(ctx, tosContract, blockNum)
if err != nil {
Expand Down Expand Up @@ -344,3 +370,14 @@ func (a *onchainAllowlist) loadStoredAllowedSenderList() {

a.update(allowedList)
}

func extractContractVersion(str string) (string, error) {
pattern := `v(\d+).(\d+).(\d+)`
re := regexp.MustCompile(pattern)

match := re.FindStringSubmatch(str)
if len(match) != 4 {
return "", fmt.Errorf("version not found in string: %s", str)
}
return match[1] + match[2] + match[3], nil
}
Loading
Loading