Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(runtime): Implement branch service #18475

Merged
merged 8 commits into from
Nov 15, 2023
Merged

Conversation

testinginprod
Copy link
Contributor

@testinginprod testinginprod commented Nov 15, 2023

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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

Summary by CodeRabbit

  • New Features

    • Implemented a new BranchService with enhanced execution methods and gas limit handling.
    • Improved gRPC reflection compatibility with updated package naming.
  • Tests

    • Added comprehensive tests for the new BranchService functionalities.
  • Documentation

    • Updated CHANGELOG to reflect the introduction of the new service implementation.
  • Refactor

    • Standardized import statements for better consistency and maintenance.
  • Chores

    • Integrated Cosmos SDK types for better error handling and service execution.

Copy link
Contributor

coderabbitai bot commented Nov 15, 2023

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.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between 1f896f6 and a251fef.
Walkthrough

Walkthrough

A new BranchService structure has been implemented in the runtime module, enhancing the core.branch.Service with additional methods for executing functions with and without a gas limit. This update includes error handling and integration with Cosmos SDK types. Additionally, the tools/hubl package has undergone a minor update, changing the gRPC reflection package naming. The x/crisis module now imports the Cosmos SDK version package, though no functional changes are noted.

Changes

File(s) Summary
CHANGELOG.md Added entry for new BranchService in runtime module.
runtime/branch.go, runtime/branch_test.go Introduced BranchService with Execute and ExecuteWithGasLimit methods, including tests and a catchOutOfGas helper function.
tools/hubl/internal/compat.go Updated gRPC reflection package naming.
x/crisis/autocli.go Imported "github.com/cosmos/cosmos-sdk/version" package.

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

unknown unknown added 2 commits November 15, 2023 13:19
# 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
@testinginprod testinginprod marked this pull request as ready for review November 15, 2023 12:33
@testinginprod testinginprod requested a review from a team as a code owner November 15, 2023 12:33
@github-actions github-actions bot removed the C:Store label Nov 15, 2023
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 5

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between e64f3bc and 4bef15f.
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 BranchService and its methods Execute and ExecuteWithGasLimit seem to follow the expected pattern for handling context branching and gas metering in the Cosmos SDK. The use of CacheContext for branching and NewGasMeter for gas limiting is consistent with Cosmos SDK patterns.
  • The Execute method correctly branches the context, executes the function, and commits the changes if there are no errors.
  • The ExecuteWithGasLimit method 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 catchOutOfGas helper function is a good use of Go's defer and recover to handle panics related to out-of-gas errors, converting them into a standard error.

However, there are a few points to consider:

CHANGELOG.md Show resolved Hide resolved
runtime/branch.go Show resolved Hide resolved
runtime/branch.go Outdated Show resolved Hide resolved
runtime/branch.go Outdated Show resolved Hide resolved
runtime/branch_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 4bef15f and 1f896f6.
Files selected for processing (1)
  • runtime/branch.go (1 hunks)
Additional comments: 1
runtime/branch.go (1)
  • 1-67: The implementation of BranchService and its methods Execute and ExecuteWithGasLimit seem to follow the intended design, providing a way to execute functions with and without a gas limit. The catchOutOfGas helper 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 catchOutOfGas function logs to os.Stderr directly (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 catchOutOfGas function re-panics if the recovered panic is not an OutOfGas error (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 ExecuteWithGasLimit method 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.

@kocubinski kocubinski self-assigned this Nov 15, 2023

var _ branch.Service = BranchService{}

type BranchService struct{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this type have a godoc?

Comment on lines +21 to +26
branchedCtx, commit := sdkCtx.CacheContext()
err := f(branchedCtx)
if err != nil {
return err
}
commit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: break it up to make it easier to grok

Suggested change
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would break this up a bit to make it easier to read

@testinginprod testinginprod added this pull request to the merge queue Nov 15, 2023
Merged via the queue into main with commit 5f36ad0 Nov 15, 2023
71 of 77 checks passed
@testinginprod testinginprod deleted the tip/runtime/branch branch November 15, 2023 18:41
@testinginprod testinginprod mentioned this pull request Dec 19, 2023
44 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants