-
Notifications
You must be signed in to change notification settings - Fork 118
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
Improve TState
#1834
base: main
Are you sure you want to change the base?
Improve TState
#1834
Conversation
internal/fetcher/fetcher_test.go
Outdated
@@ -142,12 +150,16 @@ func TestFetchSameKeysSlow(t *testing.T) { | |||
stateKeys.Add(keyBase+strconv.Itoa(k), state.Read) | |||
} | |||
txID := ids.GenerateTestID() | |||
keys := make([]string, 0, len(stateKeys)) |
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.
converting the state keys map into a keys slice seems to be a common usage pattern; do you think it would make sense to create a function that would encapsulate that ?
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.
For the tests, we can switch to generating database keys instead of generating state keys + immediately stripping their permissions. But agree that we should add a function that converts state keys into a keys slice.
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.
Done
This PR makes the following improvements related to
TState
:fetcher
now takes just the database keys that it needs to fetch, rather than state keys (i.e. database keys + it's permissions) asfetcher
is not and should not be responsible for permission validationTState
have been replaced by aScope
interface which implements both reads and permission validation requests (i.e.Has()
).TStateRecorder
has been replaced bySimulatedScope
dbtest
has been moved into its own package, following the convention in AvalancheGo.