feat(stf): change router service to extract the router at runtime rather than build time #20724
Conversation
WalkthroughThis update reengineers message and query handling within the Changes
Assessment against linked issues
Possibly related issues
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 (
|
…to tip/stf/change_routerservice
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
server/v2/stf/core_branch_service_test.go (1)
Line range hint
19-44: Ensure comprehensive coverage and check for potential improvements in testing practices.The additions to the test setup in
TestBranchServicefunction appear to cover new functionality. However, consider adding more detailed assertions to ensure that the state changes are as expected, and not just that no errors occur. Would you like assistance in enhancing these tests?server/v2/stf/stf_router.go (1)
Line range hint
65-178: Review and refactor theRouterimplementation for optimization and error handling.The
Routerstruct and its methods are crucial for routing and handling messages. Consider optimizing the message handling logic to reduce duplication and improve performance. Additionally, ensure that error messages are clear and provide enough context to help diagnose issues quickly.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (8)
- runtime/v2/builder.go (2 hunks)
- runtime/v2/module.go (1 hunks)
- server/v2/stf/core_branch_service_test.go (2 hunks)
- server/v2/stf/core_router_service.go (1 hunks)
- server/v2/stf/stf.go (11 hunks)
- server/v2/stf/stf_router.go (4 hunks)
- server/v2/stf/stf_test.go (4 hunks)
- store/rootmulti/store_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- store/rootmulti/store_test.go
Additional context used
Path-based instructions (7)
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/core_branch_service_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"server/v2/stf/stf_router.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/stf_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"server/v2/stf/stf.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (7)
server/v2/stf/core_router_service.go (2)
44-54: Implementation ofqueryRouterServicelooks clean and follows the interface specifications.The
queryRouterServicestruct is implemented correctly according to the interface specifications, and there are no apparent issues with the methods provided.
27-40: Ensure type safety and error handling inmsgRouterServicemethods.These methods cast
ctxto*executionContextwithout checking its actual type, which could lead to runtime panics ifctxis not of the expected type. Consider adding a type assertion and appropriate error handling to prevent potential runtime errors.runtime/v2/builder.go (1)
Line range hint
101-116: Validate the integration of the STF setup in theBuildmethod.The integration of the STF using
stf.NewSTFin theBuildmethod appears correct, but ensure that all parameters passed are properly initialized and that the error handling is robust enough to catch and report any issues during the STF setup.runtime/v2/module.go (1)
214-215: RefactoredQueryRouterServiceandMsgRouterServiceinitialization to be more modular and runtime-oriented.server/v2/stf/stf_test.go (2)
23-54: IntroducedaddMsgHandlerToSTFto dynamically add message handlers, enhancing test flexibility and aligning with runtime message handling improvements.
90-93: Updated test scenarios to useaddMsgHandlerToSTF, ensuring comprehensive testing of the new dynamic message handling capabilities.Also applies to: 160-162, 203-205
server/v2/stf/stf.go (1)
47-79: RefactoredNewSTFto accept dynamic router builders, enabling runtime router configuration and reducing build-time dependencies.
| func NewMsgRouterService(identity transaction.Identity) router.Service { | ||
| return msgRouterService{identity: identity} | ||
| } | ||
|
|
||
| var _ router.Service = (*msgRouterService)(nil) | ||
|
|
||
| type msgRouterService struct { | ||
| builder *MsgRouterBuilder | ||
| handler appmodulev2.Handler | ||
| // TODO(tip): the identity sits here for the purpose of disallowing modules to impersonate others (sudo). | ||
| // right now this is not used, but it serves the reminder of something that we should be eventually | ||
| // looking into. | ||
| identity []byte |
There was a problem hiding this comment.
Refactor suggestion for msgRouterService: Consider using the identity field in future implementations.
The identity field in the msgRouterService struct is currently unused. It's a good practice to avoid leaving unused code as it can lead to confusion and maintenance issues. If there are plans to use this field in the future, consider documenting the intended use more explicitly or implementing a placeholder functionality that can be expanded later.
| @@ -42,29 +44,39 @@ type STF[T transaction.Tx] struct { | |||
|
|
|||
| // NewSTF returns a new STF instance. | |||
| func NewSTF[T transaction.Tx]( | |||
| handleMsg func(ctx context.Context, msg transaction.Msg) (transaction.Msg, error), | |||
| handleQuery func(ctx context.Context, req transaction.Msg) (transaction.Msg, error), | |||
| msgRouterBuilder *MsgRouterBuilder, | |||
There was a problem hiding this comment.
nit, can we add the logger here so it closes this as well: #20663
server/v2/stf/stf.go
Outdated
| return &STF[T]{ | ||
| handleMsg: handleMsg, | ||
| handleQuery: handleQuery, | ||
| logger: nil, |
server/v2/stf/stf_router.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| gogoproto.Merge(resp, handlerResp) |
There was a problem hiding this comment.
This will panic on custom typed gogo fields when the message is backed by a go struct, for example:
func Test_CoinMerge(t *testing.T) {
src := sdk.NewInt64Coin("atom", 10)
dst := &sdk.Coin{}
require.Panics(t, func() {
gogoproto.Merge(dst, &src)
})
}Previously I had (quite horrendous)
reflect.Indirect(reflect.ValueOf(resp)).Set(reflect.Indirect(reflect.ValueOf(res)))But I don't see any other alternatives on the table for a working implementation of InvokeTyped given the constraints of gogoproto and golang. We could remove InvokeTyped and just use InvokeUntyped, forcing callers to cast the returned type themselves.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- server/v2/stf/stf_router.go (4 hunks)
- server/v2/stf/stf_router_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- server/v2/stf/stf_router_test.go
Files skipped from review as they are similar to previous changes (1)
- server/v2/stf/stf_router.go
…to tip/stf/change_routerservice
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- runtime/v2/builder.go (2 hunks)
- runtime/v2/module.go (1 hunks)
- server/v2/stf/stf.go (11 hunks)
- server/v2/stf/stf_router.go (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- runtime/v2/builder.go
- runtime/v2/module.go
- server/v2/stf/stf.go
- server/v2/stf/stf_router.go
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -4,11 +4,13 @@ | |||
| "context" | |||
| "errors" | |||
| "fmt" | |||
| "reflect" | |||
Check notice
Code scanning / CodeQL
Sensitive package import Note
Description
Before: the router was being built and being passed to modules, this created a cyclic dependency in which modules to be be instantiated required the router, which in turn was created by STF that required all the modules.
After: the router is now extracted when needed by modules as they're executing a state transition, this makes it such that building logic is decocupled from execution logic.
Also closes: #20663
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
Refactor
Tests
Chores