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

fix: feemarket blackberry security update #275

Closed
wants to merge 2 commits into from

Conversation

aljo242
Copy link
Contributor

@aljo242 aljo242 commented Aug 21, 2024

  • update x/feemarket to v1.1.0
  • update app.go setup to work with new initialization
  • fix testing suite (note the suite AuthKeeper was not being initialized properly)

Summary by CodeRabbit

  • New Features

    • Enhanced transaction processing with the integration of a BankKeeper in the NewAnteHandler.
  • Bug Fixes

    • Improved test reliability for transaction scenarios by refining mock behaviors in the test suite.
  • Documentation

    • Updated documentation to reflect the removal of FeeGrantKeeper from post-handler options.
  • Chores

    • Updated several dependencies to improve performance and maintain security.

Copy link
Contributor

coderabbitai bot commented Aug 21, 2024

Walkthrough

The recent updates significantly enhance the application's functionality by integrating a BankKeeper into the ante handler, refining test setups for decorators, and modifying fee management processes. Changes include improved test accuracy and dependency management, reflecting a focus on better handling of financial operations. Overall, these adjustments aim to streamline interactions and enhance the overall architecture of the application.

Changes

Files Change Summary
app/ante/ante.go, app/ante/ante_test.go, app/ante/suite.go Enhanced integration of BankKeeper into the ante handler and tests, improving transaction processing and test accuracy. Mocking practices improved for better dependency management.
app/app.go Removed FeeGrantKeeper from post-handler options, simplifying fee management processes.
go.mod Updated Go version and various dependencies, ensuring the module remains current and stable.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant AnteHandler
    participant BankKeeper
    participant FeeMarketCheckDecorator

    Client->>AnteHandler: Send transaction
    AnteHandler->>BankKeeper: Validate funds
    BankKeeper-->>AnteHandler: Funds valid
    AnteHandler->>FeeMarketCheckDecorator: Check fees
    FeeMarketCheckDecorator-->>AnteHandler: Fees validated
    AnteHandler-->>Client: Transaction processed
Loading

🐰 "In the meadow where bunnies bounce,
Changes bring joy, we all can pounce!
With BankKeeper's grace, our paths entwine,
In fields of code, our futures shine!
Hopping along with each refined line,
Here’s to the changes, oh how divine!" 🌼


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • 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/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 94ff7a9 and 8f1b119.

Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
Files selected for processing (5)
  • app/ante/ante.go (2 hunks)
  • app/ante/ante_test.go (5 hunks)
  • app/ante/suite.go (4 hunks)
  • app/app.go (2 hunks)
  • go.mod (6 hunks)
Additional comments not posted (25)
app/ante/ante_test.go (4)

4-4: Approved: Integration of testify/mock.

The use of testify/mock for mocking dependencies is appropriate and enhances the test setup.


103-104: Approved: Creation of test accounts.

The use of CreateTestAccounts enhances the realism of the test scenarios.


50-52: Verify: Consistent use of mocked SendCoinsFromAccountToModule.

Ensure that all instances of SendCoinsFromAccountToModule are consistently mocked across test cases.

Run the following script to verify the consistent use of the mocked method:

Also applies to: 64-65

Verification successful

Consistent Use of Mocked SendCoinsFromAccountToModule Verified

The SendCoinsFromAccountToModule method is consistently mocked across the test cases in app/ante/ante_test.go. Both instances use the same parameters and return values. No inconsistencies were found.

  • app/ante/ante_test.go: Lines where the method is mocked consistently.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent mocking of `SendCoinsFromAccountToModule`.

# Test: Search for the mocked method usage. Expect: Consistent mocking across test cases.
rg --type go -A 3 $'suite.bankKeeper.On("SendCoinsFromAccountToModule"'

Length of output: 193


Script:

#!/bin/bash
# Description: Verify consistent mocking of `SendCoinsFromAccountToModule`.

# Test: Search for the mocked method usage. Expect: Consistent mocking across test cases.
rg --type go -A 3 $'suite.bankKeeper.On\\("SendCoinsFromAccountToModule"'

Length of output: 596


110-115: Verify: Integration of additional parameters in FeeMarketCheckDecorator.

Ensure that the additional parameters (accountKeeper, bankKeeper, feeGrantKeeper) are correctly integrated and utilized in the FeeMarketCheckDecorator.

Run the following script to verify the integration of additional parameters:

app/ante/suite.go (2)

49-49: Verify: Compatibility of feemarketmocks.BankKeeper.

Ensure that the new mock implementation feemarketmocks.BankKeeper is compatible with existing tests in the suite.

Run the following script to verify the compatibility of the new mock implementation:

Verification successful

Verified: Compatibility of feemarketmocks.BankKeeper.

The feemarketmocks.BankKeeper is integrated into the test suite and is being utilized in test functions like TestMempoolDecorator. This suggests that the new mock implementation is compatible with the existing tests.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify compatibility of `feemarketmocks.BankKeeper`.

# Test: Search for `feemarketmocks.BankKeeper` usage. Expect: Compatibility with existing tests.
rg --type go -A 5 $'feemarketmocks.BankKeeper'

Length of output: 435


Script:

#!/bin/bash
# Description: Search for test functions that might use `bankKeeper` in `AnteTestSuite`.

# Find all test functions in the repository
rg --type go -A 5 'func Test' > all_tests.txt

# Search for references to `bankKeeper` within those test functions
rg --type go 'bankKeeper' --files-with-matches | xargs -I {} rg --type go -A 5 'func Test' {} > bankKeeper_tests.txt

# Output the relevant test functions
cat bankKeeper_tests.txt

Length of output: 590


69-69: Verify: Correct instantiation of feemarketmocks.NewBankKeeper.

Ensure that feemarketmocks.NewBankKeeper(t) is correctly instantiated and aligns with the new testing context.

Run the following script to verify the correctness of the new instantiation:

app/ante/ante.go (2)

35-35: Verify: Utilization of BankKeeper in HandlerOptions.

Ensure that the new BankKeeper field in HandlerOptions is correctly utilized in the NewAnteHandler constructor.

Run the following script to verify the utilization of the new field:

Verification successful

Verified: Correct utilization of BankKeeper in HandlerOptions.

The BankKeeper field is properly utilized in the NewAnteHandler constructor. It is checked for nil and used in several decorators, confirming its essential role in the handler's functionality.

  • NewAnteHandler constructor checks for BankKeeper being nil and returns an error if so.
  • BankKeeper is passed to NewDeductFeeDecorator and other decorators.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify utilization of `BankKeeper` in `HandlerOptions`.

# Test: Search for `BankKeeper` usage in `NewAnteHandler`. Expect: Correct utilization.
rg --type go -A 5 $'BankKeeper'

Length of output: 12596


67-68: Verify: Integration of options.BankKeeper in NewAnteHandler.

Ensure that options.BankKeeper is correctly integrated and utilized in the NewAnteHandler function.

Run the following script to verify the integration of options.BankKeeper:

Verification successful

Integration of options.BankKeeper is verified in NewAnteHandler.

The options.BankKeeper is properly integrated and utilized within the NewAnteHandler function, particularly in the NewDeductFeeDecorator and other related decorators, ensuring its role in bank-related operations is correctly implemented.

  • app/ante/ante.go: Usage in NewDeductFeeDecorator and other decorators confirms correct integration.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify integration of `options.BankKeeper` in `NewAnteHandler`.

# Test: Search for `options.BankKeeper` usage. Expect: Correct integration.
rg --type go -A 5 $'options.BankKeeper'

Length of output: 1963

go.mod (15)

3-3: Approved: Go version update.

The update from 1.22.3 to 1.22.6 is approved, reflecting an effort to keep the module up to date.


5-5: Approved: Toolchain version update.

The update from go1.22.4 to go1.23.0 is approved, reflecting an effort to keep the module up to date.


13-13: Verify: Compatibility of github.com/cosmos/gogoproto v1.6.0.

Ensure that the updated version v1.6.0 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:


31-31: Verify: Compatibility of google.golang.org/grpc v1.65.0.

Ensure that the updated version v1.65.0 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:


209-209: Verify: Compatibility of golang.org/x/term v0.23.0.

Ensure that the updated version v0.23.0 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:

Verification successful

No direct usage of golang.org/x/term found in the codebase.

The search did not reveal any direct usage of golang.org/x/term, suggesting that the update to v0.23.0 should not cause compatibility issues. If the dependency is used indirectly, ensure that the dependent packages are compatible with the updated version.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify compatibility of `golang.org/x/term` v0.23.0.

# Test: Search for `term` usage. Expect: Compatibility with the updated version.
rg --type go -A 5 $'term'

Length of output: 1834


207-207: Verify: Compatibility of golang.org/x/sync v0.8.0.

Ensure that the updated version v0.8.0 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:

Verification successful

Verified: Compatibility of golang.org/x/sync v0.8.0.

The update to golang.org/x/sync v0.8.0 does not affect the usage of the sync package in the codebase, specifically sync.Once, which is part of the standard library. The code is compatible with the updated version.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify compatibility of `golang.org/x/sync` v0.8.0.

# Test: Search for `sync` usage. Expect: Compatibility with the updated version.
rg --type go -A 5 $'sync'

Length of output: 476


57-57: Verify: Compatibility of github.com/skip-mev/feemarket v1.1.0.

Ensure that the updated version v1.1.0 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:


214-214: Verify: Compatibility of google.golang.org/genproto/googleapis/rpc v0.0.0-20240709173604-40e1e62336c5.

Ensure that the updated version is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:

Verification successful

Verified: Indirect Dependency Update for google.golang.org/genproto/googleapis/rpc.

The dependency is marked as indirect and is not directly used in the codebase. The update to version v0.0.0-20240709173604-40e1e62336c5 should not impact the codebase directly. Ensure that dependent libraries are compatible with this version.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify compatibility of `google.golang.org/genproto/googleapis/rpc` v0.0.0-20240709173604-40e1e62336c5.

# Test: Search for `genproto/googleapis/rpc` usage. Expect: Compatibility with the updated version.
rg --type go -A 5 $'genproto/googleapis/rpc'

Length of output: 43


Script:

#!/bin/bash
# Description: List all dependencies and their versions to check for indirect usage of `genproto/googleapis/rpc`.

# List all dependencies in go.mod and their versions
cat go.mod

Length of output: 11757


38-38: Verify: Compatibility of cosmossdk.io/core v0.11.1.

Ensure that the updated version v0.11.1 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:


203-203: Verify: Compatibility of golang.org/x/crypto v0.26.0.

Ensure that the updated version v0.26.0 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:


205-205: Verify: Compatibility of golang.org/x/net v0.28.0.

Ensure that the updated version v0.28.0 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:

Verification successful

Compatibility of golang.org/x/net v0.28.0 appears intact.

The usage of the net package in cmd/eved/testnet.go is primarily for IP parsing, which is unlikely to be affected by minor version updates. However, it is recommended to manually review the release notes of golang.org/x/net v0.28.0 for any breaking changes that could affect the codebase.

  • File: cmd/eved/testnet.go
  • Function: initTestnetFiles
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify compatibility of `golang.org/x/net` v0.28.0.

# Test: Search for `net` usage. Expect: Compatibility with the updated version.
rg --type go -A 5 $'net'

Length of output: 15117


11-11: Verify: Compatibility of github.com/cosmos/cosmos-sdk v0.50.9.

Ensure that the updated version v0.50.9 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:

Verification successful

Compatibility Verified: github.com/cosmos/cosmos-sdk v0.50.9

The update to version v0.50.9 includes bug fixes and compatibility improvements without introducing breaking changes. It should be compatible with the existing codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify compatibility of `github.com/cosmos/cosmos-sdk` v0.50.9.

# Test: Search for `cosmos-sdk` usage. Expect: Compatibility with the updated version.
rg --type go -A 5 $'cosmos-sdk'

Length of output: 17545


Script:

#!/bin/bash
# Fetch the changelog or release notes for `github.com/cosmos/cosmos-sdk` v0.50.9 to check for breaking changes or important updates.
curl -s https://api.github.com/repos/cosmos/cosmos-sdk/releases/tags/v0.50.9 | jq '.body'

Length of output: 1077


208-208: Verify: Compatibility of golang.org/x/sys v0.23.0.

Ensure that the updated version v0.23.0 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:


210-210: Verify: Compatibility of golang.org/x/text v0.17.0.

Ensure that the updated version v0.17.0 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:


70-70: Verify: Compatibility of cosmossdk.io/depinject v1.0.0.

Ensure that the updated version v1.0.0 is compatible with the codebase.

Run the following script to verify the compatibility of the updated version:

app/app.go (2)

Line range hint 484-486: Verify: Impact of FeeGrantKeeper removal from PostHandlerOptions.

Ensure that the removal of FeeGrantKeeper aligns with the intended functionality and does not affect fee management.

Run the following script to verify the impact of the removal:

Verification successful

Verification Successful: FeeGrantKeeper Removal from PostHandlerOptions

The removal of FeeGrantKeeper from PostHandlerOptions does not adversely affect the fee management functionality. It is still present and used elsewhere in the codebase, indicating its relevance for other modules.

  • FeeGrantKeeper is initialized and used in app/app.go.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify impact of `FeeGrantKeeper` removal from `PostHandlerOptions`.

# Test: Search for `FeeGrantKeeper` usage in `PostHandlerOptions`. Expect: No adverse impact.
rg --type go -A 5 $'PostHandlerOptions'

Length of output: 629


Script:

#!/bin/bash
# Description: Search for any remaining usages of `FeeGrantKeeper` in the codebase to verify its removal impact.

# Test: Search for `FeeGrantKeeper` usage in the codebase. Expect: No remaining critical usages.
rg --type go 'FeeGrantKeeper'

Length of output: 406


Line range hint 573-575: Verify: Impact of FeeGrantKeeper removal from NewPostHandler.

Ensure that the removal of FeeGrantKeeper aligns with the intended functionality and does not affect fee management.

Run the following script to verify the impact of the removal:

Verification successful

Removal of FeeGrantKeeper from NewPostHandler does not impact its functionality.

The FeeGrantKeeper is not used within the NewPostHandler function, so its removal does not affect the function's operation. However, it is still utilized elsewhere in the application, ensuring its continued relevance.

  • The removal aligns with the intended functionality for NewPostHandler.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify impact of `FeeGrantKeeper` removal from `NewPostHandler`.

# Test: Search for `FeeGrantKeeper` usage in `NewPostHandler`. Expect: No adverse impact.
rg --type go -A 5 $'NewPostHandler'

Length of output: 696


Script:

#!/bin/bash
# Search for any usage of `FeeGrantKeeper` in the codebase to verify its impact.
rg --type go 'FeeGrantKeeper'

Length of output: 406

@faddat
Copy link
Contributor

faddat commented Aug 23, 2024

thanks so much @aljo242 -- super, super appreciated!

@faddat
Copy link
Contributor

faddat commented Aug 23, 2024

Hey @aljo242 thanks so much for this PR!

Only reason I'm not merging yours is I needed to make another with a fix for a go.mod and go.sum conflict.

Thanks again, Skip FTW!

@faddat faddat closed this Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants