feat(runtime): Implement branch service #18475
Conversation
|
Warning Rate Limit Exceeded@testinginprod has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 11 minutes and 1 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per repository. WalkthroughWalkthroughA new Changes
TipsChat with CodeRabbit Bot (
|
testinginprod
changed the title
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
# Conflicts: # client/v2/go.mod # x/authz/go.mod # x/group/go.mod # x/nft/go.mod # x/params/go.mod # x/upgrade/go.mod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to filter (45)
- client/v2/go.mod
- client/v2/go.sum
- go.mod
- go.sum
- simapp/go.mod
- simapp/go.sum
- simapp/gomod2nix.toml
- tests/go.mod
- tests/go.sum
- tests/starship/tests/go.mod
- tests/starship/tests/go.sum
- x/accounts/go.mod
- x/accounts/go.sum
- x/auth/go.mod
- x/auth/go.sum
- x/authz/go.mod
- x/authz/go.sum
- x/bank/go.mod
- x/bank/go.sum
- x/circuit/go.mod
- x/circuit/go.sum
- x/distribution/go.mod
- x/distribution/go.sum
- x/evidence/go.mod
- x/evidence/go.sum
- x/feegrant/go.mod
- x/feegrant/go.sum
- x/gov/go.mod
- x/gov/go.sum
- x/group/go.mod
- x/group/go.sum
- x/mint/go.mod
- x/mint/go.sum
- x/nft/go.mod
- x/nft/go.sum
- x/params/go.mod
- x/params/go.sum
- x/protocolpool/go.mod
- x/protocolpool/go.sum
- x/slashing/go.mod
- x/slashing/go.sum
- x/staking/go.mod
- x/staking/go.sum
- x/upgrade/go.mod
- x/upgrade/go.sum
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- runtime/branch.go (1 hunks)
- runtime/branch_test.go (1 hunks)
- tools/hubl/internal/compat.go (5 hunks)
- x/crisis/autocli.go (1 hunks)
Files skipped from review due to trivial changes (2)
- tools/hubl/internal/compat.go
- x/crisis/autocli.go
Additional comments: 1
runtime/branch.go (1)
- 1-68: The implementation of
BranchServiceand its methodsExecuteandExecuteWithGasLimitseem to follow the expected pattern for handling context branching and gas metering in the Cosmos SDK. The use ofCacheContextfor branching andNewGasMeterfor gas limiting is consistent with Cosmos SDK patterns.
- The
Executemethod correctly branches the context, executes the function, and commits the changes if there are no errors.- The
ExecuteWithGasLimitmethod sets up a gas meter with the specified limit, executes the function, and commits only if there are no errors. It also correctly captures the gas used and applies it to the original context's gas meter.- The
catchOutOfGashelper function is a good use of Go'sdeferandrecoverto handle panics related to out-of-gas errors, converting them into a standard error.However, there are a few points to consider:
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- runtime/branch.go (1 hunks)
Additional comments: 1
runtime/branch.go (1)
- 1-67: The implementation of
BranchServiceand its methodsExecuteandExecuteWithGasLimitseem to follow the intended design, providing a way to execute functions with and without a gas limit. ThecatchOutOfGashelper function is a good practice for converting panics into errors, which is a common pattern in Go for handling exceptional situations gracefully.However, there are a few points to consider:
- The
catchOutOfGasfunction logs toos.Stderrdirectly (line 60). It might be more appropriate to use a structured logging approach if the rest of the application does so, to keep the logging consistent and potentially more informative.- The
catchOutOfGasfunction re-panics if the recovered panic is not anOutOfGaserror (line 61). This is a good practice as it only handles specific panics and allows others to propagate, which could be important for debugging other issues.- The
ExecuteWithGasLimitmethod applies the gas used to the original context's gas meter (line 41). This is important for keeping track of the gas usage across different parts of the application.Overall, the code seems to be well-structured and follows good practices for error handling and resource management in Go. The use of interfaces and dependency injection also suggests that the code will be testable and maintainable.
|
|
||
| var _ branch.Service = BranchService{} | ||
|
|
||
| type BranchService struct{} |
There was a problem hiding this comment.
Should this type have a godoc?
| branchedCtx, commit := sdkCtx.CacheContext() | ||
| err := f(branchedCtx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| commit() |
There was a problem hiding this comment.
nit: break it up to make it easier to grok
| branchedCtx, commit := sdkCtx.CacheContext() | |
| err := f(branchedCtx) | |
| if err != nil { | |
| return err | |
| } | |
| commit() | |
| branchedCtx, commit := sdkCtx.CacheContext() | |
| err := f(branchedCtx) | |
| if err != nil { | |
| return err | |
| } | |
| commit() |
| return nil | ||
| } | ||
|
|
||
| func (b BranchService) ExecuteWithGasLimit(ctx context.Context, gasLimit uint64, f func(ctx context.Context) error) (gasUsed uint64, err error) { |
There was a problem hiding this comment.
nit: I would break this up a bit to make it easier to read
Description
Closes: #XXXX
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
BranchServicewith enhanced execution methods and gas limit handling.Tests
BranchServicefunctionalities.Documentation
Refactor
Chores