refactor(core): remove redundant ExecMode #20322
Conversation
|
Important Review SkippedReview was skipped due to path filters Files ignored due to path filters (6)
You can disable this status message by setting the WalkthroughThe changes primarily involve replacing references to the Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- server/v2/stf/stf.go (13 hunks)
Additional Context Used
Path-based Instructions (1)
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 (9)
server/v2/stf/stf.go (9)
89-89: Ensure the correctExecModeenum value is used from thetransactionpackage.Verification successful
The usage of
ExecModeFinalizein thetransactionpackage within the fileserver/v2/stf/stf.gohas been verified. The enum is correctly defined in thecore/transaction/service.goand is used appropriately in the specified file. This aligns with the changes mentioned in the review comment.
server/v2/stf/stf.go: Correct usage ofExecModeFinalizefrom thetransactionpackage.core/transaction/service.go: Definition ofExecModeFinalize.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ExecModeFinalize` is correctly defined in the `transaction` package. # Test: Search for the enum definition. Expect: Definition found in the `transaction` package. rg --type go $'ExecModeFinalize' src/core/transaction/Length of output: 115
Script:
#!/bin/bash # Description: Search for the `ExecModeFinalize` enum definition across the entire repository to verify its existence and location. # Test: Search for the enum definition across all Go files in the repository. rg --type go 'ExecModeFinalize'Length of output: 1017
129-129: Confirm thatExecModeFinalizeis the appropriate mode fordeliverTxin this context.
155-155: Check ifExecModeused indeliverTxaligns with expected transaction execution semantics.Verification successful
The search for
ExecModein the context of transaction execution has successfully identified relevant code incore/transaction/service.go. The file contains definitions and different modes ofExecMode, includingExecModeCheck,ExecModeReCheck,ExecModeSimulate, andExecModeFinalize. This indicates thatExecModeis indeed used to define various execution semantics within transaction processing.Given this information, it appears that the use of
ExecModeindeliverTxlikely aligns with the expected transaction execution semantics, as the modes are clearly defined and seem to be part of a structured approach to transaction execution.Conclusion:
- The use of
ExecModeindeliverTxaligns with the defined execution modes in the transaction service, suggesting that it adheres to the expected transaction execution semantics.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the use of `ExecMode` in `deliverTx` aligns with the expected semantics. # Test: Search for documentation or comments explaining `ExecMode` usage in transaction execution. Expect: Clear explanation found. rg --type go $'ExecMode.*transaction execution' src/core/transaction/Length of output: 132
Script:
#!/bin/bash # Description: Find the correct directory for transaction-related files and search for `ExecMode` usage. # Step 1: Find potential directories or files related to transactions fd 'transaction' --type d # Step 2: If the correct directory is found, search for `ExecMode` usage in transaction execution within those directories fd 'transaction' --type d --exec bash -c 'rg --type go "ExecMode" {}'Length of output: 506
210-210: Validate the use ofExecModeCheckinvalidateTxto ensure it's used for preliminary checks only.
229-229: EnsureExecModeused inexecTxis consistent with the transaction execution flow.
285-285: Confirm thatExecModeused inrunTxMsgsaligns with the intended message processing logic.
452-452: Check ifExecModeSimulateis correctly used for simulating transactions inSimulate.
486-486: Verify thatExecModeSimulateis appropriately used inQueryfor simulating query transactions.
506-506: EnsureExecModeFinalizeis the correct mode forRunWithCtxin the context of genesis or initialization.Verification successful
The usage of
ExecModeFinalizein the context of genesis or initialization is confirmed by the grep output from theserver/v2/stf/stf.gofile. However, a comment in the code suggests that there might still be some uncertainty about this usage ("// TODO do we need headerinfo for genesis?"). It would be advisable to review this part of the code to ensure thatExecModeFinalizeis indeed the appropriate mode for these operations.
- File:
server/v2/stf/stf.go- Contexts:
- Line 506: Usage in potentially a genesis context (commented with TODO about the necessity of header info).
- Other usages in consensus and transaction delivery contexts.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ExecModeFinalize` is the correct mode for `RunWithCtx` in the context of genesis or initialization. # Test: Search for usage patterns of `ExecModeFinalize` in genesis or initialization contexts. Expect: Correct usage in such contexts. rg --type go $'RunWithCtx.*ExecModeFinalize' src/server/v2/stf/Length of output: 123
Script:
#!/bin/bash # Description: Check if the directory 'src/server/v2/stf/' exists and list its contents if it does. ls src/server/v2/stf/Length of output: 89
Script:
#!/bin/bash # Description: Search for Go files that might contain relevant code related to `ExecModeFinalize`. fd '\.go$' --type fLength of output: 80204
Script:
#!/bin/bash # Description: Search for the usage of `ExecModeFinalize` in the context of genesis or initialization within the 'server/v2/stf/stf.go' file. grep -C 3 "ExecModeFinalize" server/v2/stf/stf.goLength of output: 813
| @@ -153,7 +152,7 @@ func (s STF[T]) deliverTx( | |||
| ctx context.Context, | |||
| state store.WriterMap, | |||
| tx T, | |||
| execMode corecontext.ExecMode, | |||
| execMode transaction.ExecMode, | |||
There was a problem hiding this comment.
This however doesn't contain all exec mode, just exec mode available for modules. So imho it should live in stf directly
There was a problem hiding this comment.
The proposal then would be to use an internal exec modes? I think it’s fine, they can be packed as uint8 into context and the service can cast so that the exec mode can properly transit the boundary to modules.
There was a problem hiding this comment.
Yes, baseapp and stf have their internal exec modes, and modules use the transaction service
There was a problem hiding this comment.
Are you going to do this in this pR btw?
There was a problem hiding this comment.
Yea working on it today
There was a problem hiding this comment.
does this need to change to internal?
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/internal/transaction.go (1 hunks)
- server/v2/stf/stf.go (13 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/stf/stf.go
Additional Context Used
Path-based Instructions (1)
server/v2/stf/internal/transaction.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
server/v2/stf/internal/transaction.go (2)
3-3: Ensure the import path is correct and accessible.
5-18: The execution modes are well-defined and adhere to the constraints specified in the comments.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- server/v2/stf/stf.go (13 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/stf/stf.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- server/v2/stf/stf.go (13 hunks)
Additional Context Used
Path-based Instructions (1)
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 (10)
server/v2/stf/stf.go (10)
17-17: Refactor to use internal package imports.This change aligns with the PR's objective to centralize execution mode definitions, reducing redundancy and potential errors from multiple sources of truth.
156-156: Review the transition to usingtransaction.ExecModefor execution mode handling.This change is part of the PR's goal to streamline execution mode definitions and should help maintain consistency across the codebase.
211-211: Validate the transition toExecModeCheckfor transaction validation.Switching to
ExecModeCheckfor validation contexts is a logical step in refining execution control flows.
453-453: Review the use ofExecModeSimulatein simulation contexts.Using
ExecModeSimulatespecifically for simulations is a clear and appropriate separation of execution modes, enhancing clarity and maintainability.
487-487: EnsureExecModeSimulateis correctly applied in query contexts.The consistent use of
ExecModeSimulatein query operations is crucial for maintaining the integrity of the simulation environment.
506-506: Check the application ofExecModeFinalizein context creation for genesis operations.Using
ExecModeFinalizein this context is appropriate, ensuring that the state transitions are finalized correctly during genesis operations.
90-90: Ensure correct usage ofExecModeFinalize.Verification successful
The implementations of
makeContextanddeliverTxfunctions have been retrieved. Both functions useExecModeFinalizeas an argument, and there is no indication of incorrect usage based on the provided context.
makeContextfunction usesExecModeFinalizeto create an execution context.deliverTxfunction usesExecModeFinalizeto execute a transaction.Both usages appear to be consistent with their intended purposes.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ExecModeFinalize` is used correctly across all relevant functions. # Test: Search for incorrect usage of `ExecModeFinalize`. Expect: No occurrences of incorrect usage. rg --type go $'ExecModeFinalize' server/v2/stf/stf.goLength of output: 312
Script:
#!/bin/bash # Description: Retrieve the implementation of the `makeContext` function. ast-grep --lang go --pattern $'func (s $_) makeContext($_, $_, $_, $_) $_' # Description: Retrieve the implementation of the `deliverTx` function. ast-grep --lang go --pattern $'func (s $_) deliverTx($_, $_, $_, $_, $_) $_'Length of output: 3202
230-230: Confirm the appropriate use ofExecModein transaction execution.Verification successful
The context around each usage of
ExecModein theserver/v2/stf/stf.gofile indicates that it is being used appropriately within the transaction execution context. TheExecModeis consistently passed as a parameter to functions that handle transaction execution, validation, and simulation, aligning with its intended purpose.
ExecModeis used in functions likedeliverTx,execTx,runTxMsgs, andmakeContext, which are responsible for different aspects of transaction processing.- The usage of
ExecModein these contexts appears to be correct and consistent with its role in defining the execution mode for transactions.Based on the gathered information, the use of
ExecModein the transaction execution context is appropriate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that `ExecMode` is used appropriately within the transaction execution context. # Test: Search for potential misuse of `ExecMode`. Expect: Proper usage aligned with the new definitions. rg --type go $'ExecMode' server/v2/stf/stf.goLength of output: 833
Script:
#!/bin/bash # Description: Extract context around each usage of `ExecMode` to verify its correctness. # Extract 5 lines before and after each occurrence of `ExecMode` in the file to understand its usage context. rg --type go -C 5 'ExecMode' server/v2/stf/stf.goLength of output: 4481
286-286: Check for correct application ofExecModein message execution.Verification successful
To ensure the correct application of
ExecModein the context of message execution, we need to examine the context around each instance whereExecModeis used. This will help us verify if it is applied consistently and correctly.
The extracted context around each usage of
ExecModeshows that it is consistently used in various functions such asmakeContext,deliverTx,execTx, andrunTxMsgs. Each instance appears to be correctly applyingExecModein the context of message execution, ensuring that the execution mode is appropriately set for different operations.
makeContextis used to create a new execution context with the specifiedExecMode.deliverTxandexecTxfunctions useExecModeto determine the mode of transaction execution.runTxMsgsusesExecModeto handle transaction messages according to the specified execution mode.Given this consistent and correct usage, the application of
ExecModein the context of message execution appears to be properly implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct application of `ExecMode` in the context of message execution. # Test: Search for any discrepancies in the usage of `ExecMode`. Expect: Consistent and correct usage. rg --type go $'ExecMode' server/v2/stf/stf.goLength of output: 833
Script:
#!/bin/bash # Extract lines around each usage of `ExecMode` to verify its correct application. # Extract 5 lines before and after each occurrence of `ExecMode` in the file. rg --type go -C 5 'ExecMode' server/v2/stf/stf.goLength of output: 4481
130-130: Check the consistency ofExecModeFinalizeusage in transaction delivery.Verification successful
The usage of
ExecModeFinalizein theserver/v2/stf/stf.gofile is consistent and aligns with the intended transaction delivery context. Bothinternal.ExecModeFinalizeandtransaction.ExecModeFinalizeare defined and used correctly.
internal.ExecModeFinalizeis used in context creation.transaction.ExecModeFinalizeis used in transaction delivery.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of `ExecModeFinalize` in transaction delivery contexts. # Test: Search for inconsistent usage patterns. Expect: Consistent usage across the file. rg --type go $'ExecModeFinalize' server/v2/stf/stf.goLength of output: 312
Script:
#!/bin/bash # Description: Verify the definitions and usages of `internal.ExecModeFinalize` and `transaction.ExecModeFinalize` to ensure consistency. # Search for the definition of `ExecModeFinalize` in the internal package rg --type go 'ExecModeFinalize' | rg 'internal' # Search for the definition of `ExecModeFinalize` in the transaction package rg --type go 'ExecModeFinalize' | rg 'transaction'Length of output: 897
Script:
#!/bin/bash # Description: Verify the definitions of `ExecModeFinalize` in `internal` and `transaction` packages. # Search for the definition of `ExecModeFinalize` in the internal package rg --type go 'ExecModeFinalize' server/v2/stf/internal/transaction.go # Search for the definition of `ExecModeFinalize` in the transaction package rg --type go 'ExecModeFinalize' core/transaction/service.goLength of output: 165
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- core/context/context.go (1 hunks)
Additional Context Used
Path-based Instructions (1)
core/context/context.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* main: (120 commits) chore: update protoc-gen-swagger to protoc-gen-openapiv2 (#20448) ci: Add GitHub Action for go mod tidy and mocks (#20501) chore: Address linter issues (#20486) fix: wrap errors in auto CLI service registration (#20493) chore: fix comment (#20498) chore: fix the note box syntax error (#20499) feat(server/v2): introduce cometbft v2 (#20483) refactor(x/upgrade): migrate to appmodule.VersionMap (#20485) docs: code guidelines changes (#20482) feat(cosmovisor): load cosmovisor configuration from toml file (#19995) perf(math): Significantly speedup Dec quo truncate and quo Roundup (#20034) fix: Bump CometBFT versions (#20481) refactor(core): remove redundant ExecMode (#20322) feat(store/v2): pruning manager (#20430) chore: force reload sonar cloud (#20480) refactor(x/accounts): reuse calculated sum in `func safeAdd` (#20458) refactor: remove `defer` in loop (#20223) ci: remove livness test (#20474) build(deps): Bump bufbuild/buf-setup-action from 1.32.1 to 1.32.2 (#20477) chore: migrate a few diagrams to mermaid (#20473) ...
Description
The types duplicate the definitions in https://github.com/cosmos/cosmos-sdk/blob/main/core/transaction/service.go#L1
we should only have one right?
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.
I have...
Summary by CodeRabbit
New Features
Improvements
Refactor
corecontext.ExecModewithtransaction.ExecModeacross multiple functions.Documentation