Add replacement APIs for methods we intend to deprecate.#3397
Open
cody-littley wants to merge 3 commits intomainfrom
Open
Add replacement APIs for methods we intend to deprecate.#3397cody-littley wants to merge 3 commits intomainfrom
cody-littley wants to merge 3 commits intomainfrom
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3397 +/- ##
==========================================
+ Coverage 59.03% 59.13% +0.09%
==========================================
Files 2102 2101 -1
Lines 173237 173273 +36
==========================================
+ Hits 102277 102463 +186
+ Misses 62083 61933 -150
Partials 8877 8877
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
blindchaser
approved these changes
May 8, 2026
Kbhat1
approved these changes
May 8, 2026
Contributor
There was a problem hiding this comment.
AI Review:
Verdict: no real bugs or blockers. The PR is additive, the methods are not wired into production paths yet, the tests pin the intended behaviors, and locking on the underlying Tree (RWMutex + immutable nodes) is fine. Both Committer implementers in the repo (memiavl.CommitStore, composite.CompositeCommitStore) are updated; no other implementer exists.
A few things worth flagging — none are blockers but matter for the follow-on work that intends to actually use these methods:
1. EVM data is invisible through these methods in SplitWrite mode. Because composite.GetChildStoreByName only consults cosmosCommitter, and SplitWrite routes EVM changesets exclusively to FlatKV, cs.Get(EVMStoreKey, ...) returns (nil, false, nil) for data that exists in FlatKV. The author pins this in TestCompositeSplitWriteEVMReadsAreInvisible with a TODO. Fine for now ("not invoked in production"), but the deprecation goal for CommitKVStore is blocked on fixing this routing. Any caller migrating off the deprecated path for EVM keys will silently see empty results.
Nothing here should block landing. The forward-looking issue is #1 — SplitWrite + EVM reads must be solved before these methods can replace GetChildStoreByName for the EVM store.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Describe your changes and provide context
Adds new methods to the top level
Committerinterface. The goal is to eventually deprecate the childCommitKVStoreinterface, and these new methods are a step in that direction.Testing performed to validate your change
Unit tests. These methods are not yet invoked in any production pathways.