feat(stf): port simappv2 changes #20587
Conversation
WalkthroughWalkthroughThe recent updates introduce significant enhancements to the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant CoreRouterService
participant Handler
participant WriterMap
participant STF
Client->>CoreRouterService: InvokeTyped(Request)
CoreRouterService->>CoreRouterService: Lazy initialize handler
CoreRouterService->>Handler: Process message
Handler->>WriterMap: GetStateChanges()
WriterMap->>WriterMap: Recurse state changes
WriterMap->>Handler: Return state changes
Handler->>CoreRouterService: Return response
CoreRouterService->>Client: Send response
sequenceDiagram
participant STF
participant Store
participant Context
STF->>Store: setHeaderInfo()
Store-->>STF: Acknowledge
STF->>Store: getHeaderInfo()
Store-->>STF: Return header info
STF->>Context: makeContext()
Context-->>STF: Return execution context
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@testinginprod your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- server/v2/stf/branch/writer_map.go (1 hunks)
- server/v2/stf/core_router_service.go (3 hunks)
- server/v2/stf/stf.go (6 hunks)
Additional context used
Path-based instructions (3)
server/v2/stf/branch/writer_map.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/core_router_service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/stf.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
server/v2/stf/core_router_service.go
32-32: undefined: MsgRouterBuilder (typecheck)
73-73: undefined: MsgRouterBuilder (typecheck)
17-17: undefined: MsgRouterBuilder (typecheck)
64-64: undefined: MsgRouterBuilder (typecheck)
server/v2/stf/stf.go
38-38: undefined: branchFn (typecheck)
39-39: undefined: makeGasMeterFn (typecheck)
40-40: undefined: makeGasMeteredStateFn (typecheck)
558-558: undefined: branchFn (typecheck)
559-559: undefined: makeGasMeterFn (typecheck)
560-560: undefined: makeGasMeteredStateFn (typecheck)
606-606: undefined: makeGasMeterFn (typecheck)
607-607: undefined: makeGasMeteredStateFn (typecheck)
608-608: undefined: branchFn (typecheck)
Additional comments not posted (1)
server/v2/stf/stf.go (1)
593-598: Simplify context creation inmakeContext.
[REFACTOR_SUGGESTion]
The methodmakeContextis frequently called and involves multiple steps. Consider optimizing this method by caching frequently used values or reducing the complexity of the context creation process to improve performance.
| // GetStateChanges returns the state changes for all actors in the WriterMap, including all direct | ||
| // ancesotors from which this WriterMap was derived. | ||
| // See WriterMap.recurseStateChanges for more details. | ||
| // Subject to possible renaming to ensure a developer can retrieve only changes in *this* branch | ||
| // context (not ancestors) if that is desired. | ||
| // see: https://github.com/cosmos/cosmos-sdk/pull/20412#discussion_r1618771230 | ||
| func (b WriterMap) GetStateChanges() ([]store.StateChanges, error) { | ||
| sc := make([]store.StateChanges, len(b.branchedWriterState)) | ||
| for account, stateChange := range b.branchedWriterState { | ||
| kvChanges, err := stateChange.ChangeSets() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var ( | ||
| changes = make(map[string][]store.KVPair) | ||
| sc []store.StateChanges | ||
| ) | ||
| if err := b.recurseStateChanges(changes); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for account, kvPairs := range changes { |
There was a problem hiding this comment.
Consider renaming GetStateChanges to reflect its functionality more accurately.
The method name GetStateChanges might imply that it only retrieves changes from the current branch, but it also includes ancestor changes. Consider renaming it to GetAllStateChanges or similar to avoid confusion.
| func (b WriterMap) recurseStateChanges(changes map[string][]store.KVPair) error { | ||
| // depth first | ||
| if wr, ok := b.state.(WriterMap); ok { | ||
| if err := wr.recurseStateChanges(changes); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for account, stateChange := range b.branchedWriterState { | ||
| kvChanges, err := stateChange.ChangeSets() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| changes[account] = append(changes[account], kvChanges...) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Optimize the recursion in recurseStateChanges.
The recursive approach in recurseStateChanges could lead to performance issues with deep recursion. Consider using an iterative approach with a stack to manage state changes, which can be more efficient and easier to understand.
| // TODO lazy initialization is ugly and not thread safe. we don't want to check a mutex on every InvokeTyped either. | ||
| if m.handler == nil { | ||
| var err error | ||
| m.handler, err = m.builder.Build() | ||
| if err != nil { | ||
| return fmt.Errorf("cannot create queryRouter: %w", err) | ||
| } | ||
| } | ||
| // reflection is required, see https://github.com/cosmos/cosmos-sdk/pull/20349 | ||
| res, err := m.handler(ctx, req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| reflect.Indirect(reflect.ValueOf(resp)).Set(reflect.Indirect(reflect.ValueOf(res))) | ||
| return nil |
There was a problem hiding this comment.
Address thread safety and performance concerns in InvokeTyped.
The lazy initialization of m.handler in InvokeTyped is not thread-safe. This could lead to race conditions in a multi-threaded environment. Consider initializing m.handler in the constructor or using synchronization mechanisms like mutexes to ensure thread safety.
| const headerInfoPrefix = 0x37 | ||
|
|
||
| // setHeaderInfo sets the header info in the state to be used by queries in the future. | ||
| func (s STF[T]) setHeaderInfo(state store.WriterMap, headerInfo header.Info) error { | ||
| runtimeStore, err := state.GetWriter(appmanager.RuntimeIdentity) | ||
| // TODO storing header info is too low level here, stf should be stateless. | ||
| // We should have a keeper that does this. | ||
| runtimeStore, err := state.GetWriter(Identity) |
There was a problem hiding this comment.
Refactor setHeaderInfo to separate concerns.
The method setHeaderInfo is performing low-level operations that might be better handled by a dedicated component. Consider introducing a HeaderKeeper that encapsulates the logic for storing and retrieving header information, which would make STF more focused on its primary responsibilities.
There was a problem hiding this comment.
Breaks out the stf code from #20412, LGTM. Something needs to provision an stf store key, but I guess it's OK to leave that to follow on PRs too.
| runtimeStore, err := state.GetWriter(appmanager.RuntimeIdentity) | ||
| // TODO storing header info is too low level here, stf should be stateless. | ||
| // We should have a keeper that does this. | ||
| runtimeStore, err := state.GetWriter(Identity) |
There was a problem hiding this comment.
Like we talked about, this GetWriter("stf") will err unless an "stf" storekey is provided, probably by runtime/v2 if we don't want depinject infra code in stf.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
* main: refactor(x/auth): Fix system test (#20531) feat(crypto): add blst (#20296) docs: Update high level overview and introduction (#20535) refactor(x/core): remove test (#20624) feat(x/mint)!: Replace InflationCalculationFn with MintFn + simple epoch minting (#20363) docs: remove duplicate words (#20622) feat: prep for errors v2 (#20539) chore: reduce default inflation (#20606) refactor(store): add miss defer (#20602) chore: use comet api pkg instead of comet alias (#20614) chore: write gentx info to cmd.ErrOrStderr (#20616) docs: ADR 073: update to accepted and add to README.md (#20619) chore(proto): change future extracted modules version from v1.0.0 to v0.2.0 (#20600) fix: remove some duplicate words (#20605) feat(stf): port simappv2 changes (#20587) chore: bring patch changelogs to main (#20599)
Description
This PR ports simappv2 changes related to STF to main
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.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Refactor