-
Notifications
You must be signed in to change notification settings - Fork 3.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
refactor(store/v2): updates from integration #18633
Conversation
WalkthroughThe changes across the codebase involve interface removals, method additions, type redefinitions, and the introduction of new functions and types related to cryptographic proofs and store management. There's a shift towards more detailed concurrency control and logging, as well as adjustments to function signatures and logic to accommodate new features and improve existing ones. The updates reflect a significant refactor and enhancement of the store system, likely aimed at improving performance, security, and developer ergonomics. Changes
TipsChat with CodeRabbit Bot (
|
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.
Note to reviewer(s): This is a direct port from store v1 (with linting/cleanup).
func (s *Store) SetInitialVersion(v uint64) error { | ||
s.initialVersion = v | ||
|
||
// TODO(bez): Call SetInitialVersion on s.stateCommitment. |
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.
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.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- store/go.mod
- store/go.sum
Files selected for processing (7)
- store/commit_info.go (2 hunks)
- store/gas.go (1 hunks)
- store/proof.go (1 hunks)
- store/pruning/manager.go (3 hunks)
- store/root/store.go (9 hunks)
- store/root/store_test.go (4 hunks)
- store/store.go (5 hunks)
Additional comments: 27
store/commit_info.go (2)
8-13: The removal of the
CommitHeader
interface and the addition of theIsZero
method toCommitID
are consistent with the PR objectives and the provided summary.72-78: The
IsZero
method has been correctly added to theCommitID
type, providing a way to check if aCommitID
is uninitialized or default.store/gas.go (1)
- 9-12: The change from a type definition to a type alias for
Gas
is consistent with the PR objectives and summary provided. This change can affect type compatibility and inference in Go, so it's important to ensure that all usages of theGas
type are reviewed to confirm that they behave as expected with the new type alias.store/proof.go (1)
- 134-134: The error message is clear and does not leak sensitive information.
store/pruning/manager.go (4)
41-47: The addition of mutex locking in the
SetStorageOptions
method is correctly implemented to prevent race conditions when setting storage options.49-54: The addition of mutex locking in the
SetCommitmentOptions
method is correctly implemented to prevent race conditions when setting commitment options.123-124: The additional logging within the
select
block of thePrune
method provides useful debug information when storage pruning is still running and the current pruning operation is skipped.145-146: The additional logging within the
select
block of thePrune
method provides useful debug information when commitment pruning is still running and the current pruning operation is skipped.store/root/store.go (10)
9-15: The addition of the
coreheader
import is appropriate given the changes to theStore
struct and function signatures that now usecoreheader.Info
.44-50: The change of the
commitHeader
field's type in theStore
struct fromstore.CommitHeader
to*coreheader.Info
aligns with the PR objectives and summary.68-75: The
New
function signature has been updated to include additional parameters for pruning options, which is consistent with the PR objectives and summary.112-114: The addition of the
SetMetrics
function is not mentioned in the PR objectives or summary. Please ensure this addition is intentional and documented.116-123: The addition of the
SetInitialVersion
function and the removal of theSetPruningOptions
function align with the PR objectives and summary.126-129: The
GetSCStore
function signature has been modified to no longer take parameters, which is consistent with the PR objectives and summary.194-200: The
Query
function logic has been updated to use the new proof logic, which is consistent with the PR objectives and summary.280-282: The
SetCommitHeader
function signature has been changed to accept*coreheader.Info
, which is consistent with the PR objectives and summary.350-352: The comparison of
s.commitHeader.GetHeight()
has been changed touint64(s.commitHeader.Height)
in theCommit
function, which is consistent with the change in the type ofcommitHeader
.366-368: The
Commit
function now sets theTimestamp
field oflastCommitInfo
based oncommitHeader.Time
, which is consistent with the newcommitHeader
type.store/root/store_test.go (4)
12-16: The addition of the
"cosmossdk.io/store/v2/pruning"
import is consistent with the PR objectives and the summary provided.39-39: The change to the
New
function call to include additional pruning options is consistent with the PR objectives and the summary provided.56-56: The update to the
GetSCStore
method call to remove thestoreKey
parameter is consistent with the PR objectives and the summary provided.90-92: The change in variable access from
result.Proof
toresult.Proof.Proof
aligns with the PR objectives and the summary provided.store/store.go (5)
3-8: The changes in the import paths reflect the updates mentioned in the summary, aligning with the new proof logic and metrics integration.
20-27: The removal of the
storeKey
parameter from theGetSCStore
method aligns with the summary and indicates a change in how SC stores are accessed, potentially simplifying the interface.56-65: The addition of
SetInitialVersion
,SetCommitHeader
, andLastCommitID
methods to theRootStore
interface is consistent with the summary and suggests enhancements to the store's versioning and commit header management.78-87: The addition of the
SetMetrics
method and theRootStore
interface now extendingio.Closer
are in line with the summary, indicating an enhancement in metrics handling and resource management.177-180: The change in the
Proof
field type within theQueryResult
struct fromics23.CommitmentProof
toCommitmentOp
is consistent with the summary and reflects the integration of new proof logic.
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
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/proof.go (1 hunks)
Additional comments: 4
store/proof.go (4)
75-75: Given the previous decision to include detailed error information, this is consistent with the current approach. However, it's important to ensure that this does not lead to unintended exposure of sensitive data.
84-84: As per the previous decision, detailed error messages will be maintained. Ensure that this aligns with the overall security policy regarding error logging.
90-90: The detailed error message is consistent with the previous decision to include such information. Confirm that this is acceptable within the context of the application's security practices.
1-98: The implementation of the
CommitmentOp
struct and its methods appears to be correct and well-structured, following good Go practices with clear factory methods and error handling.
store/proof.go
Outdated
// CommitmentOpDecoder takes a ProofOp and attempts to decode it into a CommitmentOp | ||
// ProofOperator. The proofOp.Data is just a marshaled CommitmentProof. The Key | ||
// of the CommitmentOp is extracted from the unmarshalled proof. | ||
func CommitmentOpDecoder(pop cmtcrypto.ProofOp) (merkle.ProofOperator, error) { |
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.
oh sorry this could of all stayed, but we only return our own type.
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.
But it relies on cmtcrypto.ProofOp
as an argument. What exactly do you propose?
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.
need to open the code editor, i was playing with this code before to avoid the comet dependency but still be able to prove. Seems like the code you added does that anyways.
* fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * init * fix template * rename * Update telemetry/metrics.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Revert "Update telemetry/metrics.go" This reverts commit 52d7b96. * refactor(store/v2): updates from integration (cosmos#18633) * nits from bez * changelog * Update telemetry/metrics.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * update 51 app config * v50 too * fix * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * lint and gomod tidy * fix build * revert script change --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dreamweaverxyz <[email protected]> Co-authored-by: Pioua <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Marko <[email protected]>
* fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * init * fix template * rename * Update telemetry/metrics.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Revert "Update telemetry/metrics.go" This reverts commit 52d7b96. * refactor(store/v2): updates from integration (cosmos#18633) * nits from bez * changelog * Update telemetry/metrics.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * update 51 app config * v50 too * fix * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * lint and gomod tidy * fix build * revert script change --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dreamweaverxyz <[email protected]> Co-authored-by: Pioua <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Marko <[email protected]>
* feat: secp256k1 public key constant time (cosmos#18026) Signed-off-by: bizk <[email protected]> * chore: Fixed changelog duplicated items (cosmos#18628) * adr: Un-Ordered Transaction Inclusion (cosmos#18553) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> * docs: lint ADR-070 (cosmos#18634) * fix(baseapp)!: postHandler should run regardless of result (cosmos#18627) * docs: fix typos in adr-007-specialization-groups.md (cosmos#18635) * chore: alphabetize labels (cosmos#18640) * docs(x/circuit): add note on ante handler (cosmos#18637) Co-authored-by: Aleksandr Bezobchuk <[email protected]> * fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * refactor(store/v2): updates from integration (cosmos#18633) * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(store/v2): snapshot manager (cosmos#18458) * chore(client/v2): fix typos in the README.md (cosmos#18657) * fix(baseapp): protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654) Co-authored-by: unknown unknown <unknown@unknown> * chore: fix several minor typos (cosmos#18660) * chore(tools/confix/cmd): fix typo in view.go (cosmos#18659) * refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655) * feat(accounts): use gogoproto API instead of protov2. (cosmos#18653) Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651) * build(deps): Bump actions/stale from 8 to 9 (cosmos#18656) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(docs): fix typos & wording in docs (cosmos#18667) * chore: fix several typos. (cosmos#18666) * feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Marko <[email protected]> * feat(store/v2): add SetInitialVersion in SC (cosmos#18665) * feat(client/keys): support display discreetly for `keys add` (cosmos#18663) Co-authored-by: Julien Robert <[email protected]> * ci: add misspell action (cosmos#18671) * chore: typos fix by misspell-fixer (cosmos#18683) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> * chore: add v0.50.2 changelog to main (cosmos#18682) * build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor(bank): remove .String() calls (cosmos#18175) Co-authored-by: Facundo <[email protected]> * ci: use codespell instead of misspell-fixer (cosmos#18686) Co-authored-by: Marko <[email protected]> * feat(gov): add proposal types and spam votes (cosmos#18532) * feat(accounts): use account number as state prefix for account state (cosmos#18664) Co-authored-by: unknown unknown <unknown@unknown> * chore: typos fixes by cosmos-sdk bot (cosmos#18689) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: marbar3778 <[email protected]> * feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688) * refactor: remove panic usage in keeper methods (cosmos#18636) * ci: rename pr name in misspell job (cosmos#18693) Co-authored-by: Marko <[email protected]> * build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(client/keys): support display discreetly for keys export (cosmos#18684) * feat(x/gov): better gov genesis validation (cosmos#18707) --------- Signed-off-by: bizk <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Carlos Santiago Yanzon <[email protected]> Co-authored-by: yihuang <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Akaonetwo <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: dreamweaverxyz <[email protected]> Co-authored-by: Pioua <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cool-developer <[email protected]> Co-authored-by: leonarddt05 <[email protected]> Co-authored-by: testinginprod <[email protected]> Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: Sukey <[email protected]> Co-authored-by: axie <[email protected]> Co-authored-by: Luke Ma <[email protected]> Co-authored-by: Emmanuel T Odeke <[email protected]> Co-authored-by: 0xn4de <[email protected]> Co-authored-by: hattizai <[email protected]> Co-authored-by: Devon Bear <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Halimao <[email protected]> Co-authored-by: Cosmos SDK <[email protected]> Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Facundo <[email protected]> Co-authored-by: Likhita Polavarapu <[email protected]>
Description
Some minor changes that were introduced in the attempt to integrate store v2 into the framework. Most notably introducing proof logic and a bit of cleanup.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Search
class and user interface components.IsZero
method for commit ID checks.SetMetrics
function for enhanced metric tracking.Enhancements
Prune
method for better debug information.Refactor
Gas
type declaration for improved type compatibility.Store
struct to use a new header type for commit information.Bug Fixes
Documentation
Style
Tests