feat(server/v2): add SimulateWithState to AppManager #22335
Conversation
There is already the QueryWithState method to query against a modified state, but the Simulate analog was missing. This is useful in particular when a server component receives different transactions at different times, and wants to ensure that subsequent transactions would be valid to apply after the prior transactions. In other words, the first received transaction can be validated with Simulate; then the returned corestore.WriterMap is used to seed the input to the following call to SimulateWithState; and then the returned corestore.WriterMap is used for all following SimulateWithState calls. While it is currently impossible for SimulateWithState to return an error, an error value is in the return signature for symmetry with other AppManager methods and to allow the possibility of returned errors in the future. There were no existing tests in the server/v2/appmanager package, to be expanded to include test coverage for the new method.
mark-rushakoff
requested review from
testinginprod,
facundomedica and
a team
as code owners
📝 WalkthroughWalkthroughThe changes introduce a new method, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:server/v2
|
@mark-rushakoff 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: 1
🧹 Outside diff range and nitpick comments (1)
server/v2/appmanager/appmanager.go (1)
44-46: LGTM with a minor documentation enhancement suggestion.The method signature and placement are well-designed. Consider enhancing the documentation to include a usage example or common use case, similar to how QueryWithState's documentation explains its practical application.
// SimulateWithState runs validation and execution flow of a Tx, // using the provided state instead of loading the latest state from the underlying database. +// For example, it can be used to validate subsequent transactions by passing the WriterMap +// from a previous simulation as the state parameter. SimulateWithState(ctx context.Context, state corestore.ReaderMap, tx T) (server.TxResult, corestore.WriterMap, error)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- server/v2/appmanager/appmanager.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/appmanager/appmanager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
server/v2/appmanager/appmanager.go (1)
203-203: Address or remove the TODO comment.The TODO comment about checking if gas limit is handled in the antehandler appears in both
SimulateandSimulateWithState. This should be investigated and either addressed or removed to maintain code clarity.Let's check if the antehandler handles the gas limit:
| // SimulateWithState runs validation and execution flow of a Tx, | ||
| // using the provided state instead of loading the latest state from the underlying database. | ||
| func (a appManager[T]) SimulateWithState(ctx context.Context, state corestore.ReaderMap, tx T) (server.TxResult, corestore.WriterMap, error) { | ||
| result, cs := a.stf.Simulate(ctx, state, a.config.SimulationGasLimit, tx) // TODO: check if this is done in the antehandler | ||
| return result, cs, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider explicit error handling for invalid state.
While the PR objectives mention that errors aren't currently returned, consider adding validation for the provided state parameter to prevent potential panics or undefined behavior.
func (a appManager[T]) SimulateWithState(ctx context.Context, state corestore.ReaderMap, tx T) (server.TxResult, corestore.WriterMap, error) {
+ if state == nil {
+ return server.TxResult{}, nil, errors.New("provided state cannot be nil")
+ }
result, cs := a.stf.Simulate(ctx, state, a.config.SimulationGasLimit, tx)
return result, cs, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SimulateWithState runs validation and execution flow of a Tx, | |
| // using the provided state instead of loading the latest state from the underlying database. | |
| func (a appManager[T]) SimulateWithState(ctx context.Context, state corestore.ReaderMap, tx T) (server.TxResult, corestore.WriterMap, error) { | |
| result, cs := a.stf.Simulate(ctx, state, a.config.SimulationGasLimit, tx) // TODO: check if this is done in the antehandler | |
| return result, cs, nil | |
| } | |
| // SimulateWithState runs validation and execution flow of a Tx, | |
| // using the provided state instead of loading the latest state from the underlying database. | |
| func (a appManager[T]) SimulateWithState(ctx context.Context, state corestore.ReaderMap, tx T) (server.TxResult, corestore.WriterMap, error) { | |
| if state == nil { | |
| return server.TxResult{}, nil, errors.New("provided state cannot be nil") | |
| } | |
| result, cs := a.stf.Simulate(ctx, state, a.config.SimulationGasLimit, tx) // TODO: check if this is done in the antehandler | |
| return result, cs, nil | |
| } |
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
There is already the QueryWithState method to query against a modified state, but the Simulate analog was missing.
This is useful in particular when a server component receives different transactions at different times, and wants to ensure that subsequent transactions would be valid to apply after the prior transactions.
In other words, the first received transaction can be validated with Simulate; then the returned corestore.WriterMap is used to seed the input to the following call to SimulateWithState; and then the returned corestore.WriterMap is used for all following SimulateWithState calls.
While it is currently impossible for SimulateWithState to return an error, an error value is in the return signature for symmetry with other AppManager methods and to allow the possibility of returned errors in the future.
There were no existing tests in the server/v2/appmanager package, to be expanded to include test coverage for the new method.
Summary by CodeRabbit