-
Notifications
You must be signed in to change notification settings - Fork 19
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
refactor: database and checkpoint dependencies #421
Conversation
It declares the expected db interface by the checkpoint manager
…tabase interface declared in checkpoint package
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.
Hey, thanks for this! This generally looks really helpful, just one tiny bug but otherwise lgtm
database/kvstore.go
Outdated
usedDB := "Redis" | ||
if cfg.RedisURL != "" { | ||
dbConnect = func() (keyValueDatabase, error) { return NewRedisDatabase(logger, cfg.StorageLocation, relayerIDs) } | ||
usedDB = "JSON" |
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.
The usedDB
values here are flipped.
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.
LGTM, but @cam-schultz should take a look at this as well :)
This looks out dated, closing it |
Why this should be merged
It closes #418
This PR is a proposition and open discussion for the refactoring of the database and the checkpoint packages.
How this works
This declares the DB interface that checkpoint manager is expecting.
It hides database complexity and implementation details.
Checkpoint package has no more dependency on database package, database has dependency on checkpoint.
RelayerID is a "business object", the declaration has been moved into the relayer package.
How this was tested
Existing tests have been adapted.
How is this documented