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: integrate x/feemarket #250

Merged
merged 18 commits into from
Jul 5, 2024
Merged

feat: integrate x/feemarket #250

merged 18 commits into from
Jul 5, 2024

Conversation

tuantran1702
Copy link
Contributor

@tuantran1702 tuantran1702 commented Jun 21, 2024

Description

Partially addresses #245

Summary by CodeRabbit

  • New Features

    • Introduced a Fee Market module to manage transaction fees, including denomination resolution and fee calculation.
  • Bug Fixes

    • Adjusted static check warnings for bank and staking parameter key tables.
  • Tests

    • Added tests for fee handling logic with various scenarios and denominations.
  • Upgrades

    • Implemented new upgrades to include the Fee Market module and associated configurations.

Copy link
Contributor

coderabbitai bot commented Jun 21, 2024

Walkthrough

The changes introduce a feemarket module to the application, enhancing fee handling and transaction processing. New structures, handlers, and initializations related to the fee market are included. Additionally, new test cases, error definitions, and upgrade mechanisms are provided to support this module. This lays the groundwork for improved fee resolution and processing capabilities within the app.

Changes

File(s) Change Summary
app/ante.go Added feemarket imports, setup FeeMarketKeeper and AccountKeeper in HandlerOptions, introduced DenomResolverImpl, and updated ante handler construction.
app/app.go Updated imports, modified maccPerms, added FeeMarketKeeper, included feemarket module in several functions, and updated posthandler setup.
app/upgrades.go Removed //nolint:staticcheck directive and included FeeMarketKeeper in AppKeepers, added v1 upgrade.
app/ante/ante_test.go Added tests for fee handling logic, verifying different fee amounts and denomination scenarios.
app/ante/errors.go Introduced error definitions and functions for Ante module, focusing on missing keepers and configurations.
app/upgrades/v1/constants.go Added constant Upgrade with details and listed modules for upgrades in StoreUpgrades.
app/upgrades/v1/upgrades.go Added functions to create upgrade handlers and configure the fee market module.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant AnteHandler
    participant FeeMarketKeeper
    participant DenomResolver
    
    User->>AnteHandler: Submit Transaction
    AnteHandler->>FeeMarketKeeper: Validate Transaction Fee
    FeeMarketKeeper->>DenomResolver: Resolve Denomination
    DenomResolver-->>FeeMarketKeeper: Return Resolution
    FeeMarketKeeper-->>AnteHandler: Validation Result
    AnteHandler-->>User: Transaction Status
Loading

Poem

Amidst the code, a market rises bright,
Where fees grow fair and rightly paid in light,
With handlers deft and resolvers keen,
Transactions flow as smooth as a dream.
Upgrades now in place, the future's clear,
In the realm of fees, there's naught to fear. 🎉


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

@faddat faddat requested review from minhngoc274 and hoank101 July 2, 2024 06:23
@faddat
Copy link
Contributor

faddat commented Jul 2, 2024

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Jul 2, 2024

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@faddat
Copy link
Contributor

faddat commented Jul 2, 2024

In upgrades.go, there are a number of deprecated calls. Are these needed?

I'd like to get this PR landed asap if possible.

Copy link
Contributor

coderabbitai bot commented Jul 2, 2024

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

coderabbitai bot commented Jul 2, 2024

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 302941b and 7543a9b.

Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
Files selected for processing (4)
  • app/ante.go (2 hunks)
  • app/app.go (15 hunks)
  • app/upgrades.go (1 hunks)
  • go.mod (9 hunks)
Additional context used
golangci-lint
app/upgrades.go

83-83: SA1019: banktypes.ParamKeyTable is deprecated: ParamKeyTable for bank module.

(staticcheck)


85-85: SA1019: stakingtypes.ParamKeyTable is deprecated: now params can be accessed on key 0x51 on the staking store. ParamTable for staking module

(staticcheck)

GitHub Check: lint
app/upgrades.go

[failure] 83-83:
SA1019: banktypes.ParamKeyTable is deprecated: ParamKeyTable for bank module. (staticcheck)


[failure] 85-85:
SA1019: stakingtypes.ParamKeyTable is deprecated: now params can be accessed on key 0x51 on the staking store. ParamTable for staking module (staticcheck)

Additional comments not posted (13)
app/ante.go (2)

34-35: Verify the initialization of FeeMarketKeeper and AccountKeeper.

Ensure that FeeMarketKeeper and AccountKeeper are correctly initialized and used in the application.

Verification successful

Initialization of FeeMarketKeeper and AccountKeeper is verified successfully.

  • FeeMarketKeeper is initialized in app/app.go as app.FeeMarketKeeper = feemarketkeeper.NewKeeper(...).
  • AccountKeeper is initialized in app/app.go as app.AccountKeeper = authkeeper.NewAccountKeeper(...).

Both keepers are utilized extensively throughout the codebase, confirming their proper usage.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the initialization and usage of FeeMarketKeeper and AccountKeeper.

# Test: Search for the initialization of FeeMarketKeeper and AccountKeeper.
rg --type go $'FeeMarketKeeper'
rg --type go $'AccountKeeper'

Length of output: 5805


10-10: Verify the initialization of feemarketante package.

Ensure that the feemarketante package is correctly initialized and used in the application.

Verification successful

Initialization and usage of feemarketante package verified.

The feemarketante package's FeeMarketKeeper and AccountKeeper fields are correctly initialized and used in the application.

  • FeeMarketKeeper initialization: app/app.go
  • AccountKeeper initialization: app/app.go
  • Usage checks: app/ante.go
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the initialization and usage of the feemarketante package.

# Test: Search for the initialization of the feemarketante package.
rg --type go $'feemarketante'

Length of output: 232



Script:

#!/bin/bash
# Description: Verify the initialization and usage of the FeeMarketKeeper and AccountKeeper fields.

# Test: Search for the initialization of the FeeMarketKeeper field.
rg --type go 'FeeMarketKeeper\s*=\s*'

# Test: Search for the initialization of the AccountKeeper field.
rg --type go 'AccountKeeper\s*=\s*'

# Test: Search for the usage of the FeeMarketKeeper field.
rg --type go 'FeeMarketKeeper\.'

# Test: Search for the usage of the AccountKeeper field.
rg --type go 'AccountKeeper\.'

Length of output: 877

go.mod (4)

30-30: Verify the compatibility of new dependencies.

Ensure that the new dependencies, including google.golang.org/genproto, are compatible with the existing codebase.


16-16: Verify the compatibility of updated dependencies.

Ensure that the updated dependencies, including github.com/dvsekhvalnov/jose2go, are compatible with the existing codebase.


56-56: Verify the integration of github.com/skip-mev/feemarket.

Ensure that the integration of github.com/skip-mev/feemarket is correctly implemented and does not introduce any issues.

Verification successful

Integration of github.com/skip-mev/feemarket verified.

The module github.com/skip-mev/feemarket is correctly imported and utilized in the codebase without any apparent issues.

  • Files involved:
    • app/ante.go
    • app/app.go
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the integration of github.com/skip-mev/feemarket.

# Test: Check for any conflicts or issues with the integration of the feemarket module.
go mod tidy
go mod verify

Length of output: 169



Script:

#!/bin/bash
# Description: Verify the integration of github.com/skip-mev/feemarket by searching for its usage in the codebase.

# Search for the import statement of github.com/skip-mev/feemarket in the codebase
rg 'github.com/skip-mev/feemarket' --glob '**/*.go'

# Verify if the module is being used in the codebase
rg 'feemarket' --glob '**/*.go'

Length of output: 2217


61-66: Verify the compatibility of updated cloud dependencies.

Ensure that the updated cloud dependencies, including cloud.google.com/go, are compatible with the existing codebase.

app/app.go (7)

54-58: Verify the necessity of new imports.

Ensure that the newly added imports from the feemarket module and related packages are necessary and correctly used in the code.

Verification successful

Verified the necessity of new imports.

The newly added imports from the feemarket module and related packages are necessary and correctly used in the code.

  • feemarketapp is used for post handler options and creation.
  • feemarket is used for app module initialization.
  • feemarketkeeper is used for the FeeMarketKeeper.
  • feemarketpost is used for creating a fee market deduct decorator.
  • feemarkettypes is used for module names, store keys, and the test denom resolver.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the necessity of new imports from feemarket module.

# Test: Search for the usage of new imports. Expect: Usage of all new imports.
rg --type go $'feemarket|feemarketkeeper|feemarketpost|feemarkettypes'

Length of output: 1775


257-257: Verify the addition of FeeMarketKeeper.

Ensure that the addition of FeeMarketKeeper as a pointer to the EveApp struct is consistent and correctly integrated.

Verification successful

Verify the imports and integration of FeeMarketKeeper.

Ensure that the addition of FeeMarketKeeper as a pointer to the EveApp struct is consistent, all necessary imports are included, and there are no related errors or inconsistencies.


The addition of FeeMarketKeeper is consistent and correctly integrated.

  • The import statements for feemarketkeeper are present.
  • The usage of FeeMarketKeeper is consistent across the app package.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the addition and integration of FeeMarketKeeper.

# Test: Search for the usage of FeeMarketKeeper in EveApp. Expect: Correct integration.
rg --type go $'FeeMarketKeeper'

Length of output: 719



Script:

#!/bin/bash
# Description: Verify the imports and integration of FeeMarketKeeper.

# Test: Search for import statements related to FeeMarketKeeper.
rg --type go 'import' -A 5

# Test: Check for any related errors or inconsistencies in the usage of FeeMarketKeeper.
rg --type go 'feemarketkeeper' -A 5

Length of output: 4929


1083-1084: Verify the addition of FeeMarketKeeper and AccountKeeper to HandlerOptions.

Ensure that the addition of FeeMarketKeeper and AccountKeeper to HandlerOptions in the setAnteHandler function is correct and necessary.

Verification successful

Verified the addition of FeeMarketKeeper and AccountKeeper to HandlerOptions.

The addition of FeeMarketKeeper and AccountKeeper to HandlerOptions in the setAnteHandler function is correct and necessary.

  • HandlerOptions is defined in app/ante.go.
  • FeeMarketKeeper and AccountKeeper are used appropriately within the codebase.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the addition of FeeMarketKeeper and AccountKeeper to HandlerOptions.

# Test: Search for the usage of FeeMarketKeeper and AccountKeeper in HandlerOptions. Expect: Correct usage.
rg --type go $'HandlerOptions.*FeeMarketKeeper|HandlerOptions.*AccountKeeper'

Length of output: 78



Script:

#!/bin/bash
# Description: Verify the definition and usage of HandlerOptions, FeeMarketKeeper, and AccountKeeper.

# Find the definition of HandlerOptions
rg --type go 'type HandlerOptions struct'

# Check for the usage of FeeMarketKeeper and AccountKeeper in the codebase
rg --type go 'FeeMarketKeeper'
rg --type go 'AccountKeeper'

# Check for the usage of setAnteHandler to understand its context
rg --type go 'setAnteHandler'

Length of output: 6147


215-216: Verify the added permissions for feemarket module.

Ensure that the added permissions for feemarkettypes.ModuleName and feemarkettypes.FeeCollectorName align with the requirements of the feemarket module.

Verification successful

Verification successful.

The added permissions for feemarkettypes.ModuleName and feemarkettypes.FeeCollectorName align with the requirements and common patterns observed in other modules.

  • app/app.go: Lines 215-216
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the added permissions for feemarket module.

# Test: Search for the usage of feemarket module names. Expect: Correct usage and permissions.
rg --type go $'feemarkettypes.ModuleName|feemarkettypes.FeeCollectorName'

Length of output: 332



Script:

#!/bin/bash
# Description: Gather more information on the usage and requirements of the feemarket module names.

# Search for the definition and usage of feemarkettypes.ModuleName and feemarkettypes.FeeCollectorName in the codebase
rg --type go 'feemarkettypes.ModuleName|feemarkettypes.FeeCollectorName'

# Search for the definition and usage of authtypes.Burner in the codebase to understand its implications
rg --type go 'authtypes.Burner'

Length of output: 1062


607-608: Verify the initialization and configuration of FeeMarketKeeper.

Ensure that the initialization and configuration of FeeMarketKeeper in the NewEveApp function is correct.

Verification successful

Verify the initialization and configuration of FeeMarketKeeper.

The initialization of FeeMarketKeeper in app/app.go is found. Ensure the parameters passed to feemarketkeeper.NewKeeper are correct and align with the application architecture.

  • Verify the definitions and expected types of appCodec, keys[feemarkettypes.StoreKey], app.AccountKeeper, feemarkettypes.TestDenomResolver, and authtypes.NewModuleAddress(govtypes.ModuleName).String().

Verify the initialization and configuration of FeeMarketKeeper.

The initialization of FeeMarketKeeper in app/app.go is found. Ensure the parameters passed to feemarketkeeper.NewKeeper are correct and align with the application architecture.

  • Verify the definitions and expected types of appCodec and app.AccountKeeper.
  • Verify the initialization of keys[feemarkettypes.StoreKey], feemarkettypes.TestDenomResolver, and authtypes.NewModuleAddress.

Initialization and configuration of FeeMarketKeeper verified.

The parameters passed to feemarketkeeper.NewKeeper are correctly initialized and configured. The definitions and types align with the application architecture.

  • appCodec is initialized as codec.NewProtoCodec(interfaceRegistry).
  • keys[feemarkettypes.StoreKey] is correctly referenced.
  • app.AccountKeeper is initialized using authkeeper.NewAccountKeeper.
  • feemarkettypes.TestDenomResolver is correctly referenced.
  • authtypes.NewModuleAddress(govtypes.ModuleName).String() is correctly used.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the initialization and configuration of FeeMarketKeeper.

# Test: Search for the initialization and configuration of FeeMarketKeeper. Expect: Correct initialization and configuration.
rg --type go $'FeeMarketKeeper = feemarketkeeper.NewKeeper'

Length of output: 275



Script:

#!/bin/bash
# Verify the definitions and expected types of the parameters passed to feemarketkeeper.NewKeeper

# Search for the definition and type of appCodec
rg --type go 'var appCodec'

# Search for the initialization of keys[feemarkettypes.StoreKey]
rg --type go 'keys\[feemarkettypes.StoreKey\]'

# Search for the definition and type of app.AccountKeeper
rg --type go 'var AccountKeeper'

# Search for the definition of feemarkettypes.TestDenomResolver
rg --type go 'feemarkettypes.TestDenomResolver'

# Search for the definition of authtypes.NewModuleAddress
rg --type go 'authtypes.NewModuleAddress'

Length of output: 2954



Script:

#!/bin/bash
# Verify the definitions and expected types of the parameters passed to feemarketkeeper.NewKeeper

# Search for the definition and type of appCodec
rg --type go 'appCodec'

# Search for the definition and type of app.AccountKeeper
rg --type go 'app.AccountKeeper'

# Search for the initialization of keys[feemarkettypes.StoreKey]
rg --type go 'keys\[feemarkettypes.StoreKey\]'

# Search for the initialization of feemarkettypes.TestDenomResolver
rg --type go 'feemarkettypes.TestDenomResolver'

# Search for the initialization of authtypes.NewModuleAddress
rg --type go 'authtypes.NewModuleAddress'

Length of output: 11199


1096-1107: Verify the addition of FeeMarketKeeper to PostHandlerOptions.

Ensure that the addition of FeeMarketKeeper to PostHandlerOptions in the setPostHandler function is correct and necessary.


1346-1370: Verify the implementation and integration of NewPostHandler.

Ensure that the NewPostHandler function for post-processing with fee deduction is correctly implemented and integrated.

app/upgrades.go Outdated
@@ -80,9 +80,9 @@
case authtypes.ModuleName:
keyTable = authtypes.ParamKeyTable() //nolint:staticcheck
case banktypes.ModuleName:
keyTable = banktypes.ParamKeyTable() //nolint:staticcheck
keyTable = banktypes.ParamKeyTable()
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using deprecated banktypes.ParamKeyTable.

The method banktypes.ParamKeyTable is deprecated. Consider using the recommended alternatives.

- keyTable = banktypes.ParamKeyTable()
+ // Use the recommended alternative for banktypes.ParamKeyTable
+ keyTable = banktypes.NewParamKeyTable()

Committable suggestion was skipped due to low confidence.

Tools
golangci-lint

83-83: SA1019: banktypes.ParamKeyTable is deprecated: ParamKeyTable for bank module.

(staticcheck)

GitHub Check: lint

[failure] 83-83:
SA1019: banktypes.ParamKeyTable is deprecated: ParamKeyTable for bank module. (staticcheck)

app/upgrades.go Outdated
case stakingtypes.ModuleName:
keyTable = stakingtypes.ParamKeyTable() //nolint:staticcheck
keyTable = stakingtypes.ParamKeyTable()
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using deprecated stakingtypes.ParamKeyTable.

The method stakingtypes.ParamKeyTable is deprecated. Consider using the recommended alternatives.

- keyTable = stakingtypes.ParamKeyTable()
+ // Use the recommended alternative for stakingtypes.ParamKeyTable
+ keyTable = stakingtypes.NewParamKeyTable()
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.

Suggested change
keyTable = stakingtypes.ParamKeyTable()
// Use the recommended alternative for stakingtypes.ParamKeyTable
keyTable = stakingtypes.NewParamKeyTable()
Tools
golangci-lint

85-85: SA1019: stakingtypes.ParamKeyTable is deprecated: now params can be accessed on key 0x51 on the staking store. ParamTable for staking module

(staticcheck)

GitHub Check: lint

[failure] 85-85:
SA1019: stakingtypes.ParamKeyTable is deprecated: now params can be accessed on key 0x51 on the staking store. ParamTable for staking module (staticcheck)

app/app.go Fixed Show fixed Hide fixed
app/app.go Fixed Show fixed Hide fixed
"github.com/cosmos/cosmos-sdk/types/module"
)

// TODO: Add any additional upgrades here

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
app/ante/ante.go Fixed Show fixed Hide fixed
app/ante/ante.go Fixed Show fixed Hide fixed
app/ante/ante.go Fixed Show fixed Hide fixed
@tuantran1702 tuantran1702 mentioned this pull request Jul 4, 2024
4 tasks
@tuantran1702 tuantran1702 marked this pull request as ready for review July 4, 2024 13:54
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: 3

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7543a9b and 635809c.

Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
Files selected for processing (8)
  • app/ante/ante.go (1 hunks)
  • app/ante/ante_test.go (1 hunks)
  • app/ante/suite.go (1 hunks)
  • app/app.go (15 hunks)
  • app/upgrades/types.go (2 hunks)
  • app/upgrades/v1/constants.go (1 hunks)
  • app/upgrades/v1/upgrades.go (1 hunks)
  • go.mod (10 hunks)
Files skipped from review as they are similar to previous changes (1)
  • go.mod
Additional context used
GitHub Check: devskim
app/upgrades/v1/upgrades.go

[notice] 16-16: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment

app/ante/ante.go

[notice] 137-137: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment


[notice] 105-105: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment


[notice] 103-103: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment

Additional comments not posted (30)
app/upgrades/v1/constants.go (4)

1-1: Approved: Package declaration.

The package declaration is correct.


3-8: Approved: Imports.

The imports are correctly included and necessary for the functionality.


10-13: Approved: Constants.

The constants are correctly defined.


15-23: Approved: Upgrade configuration.

The Upgrade variable is correctly configured to include the feemarkettypes module.

app/upgrades/v1/upgrades.go (4)

1-1: Approved: Package declaration.

The package declaration is correct.


3-14: Approved: Imports.

The imports are correctly included and necessary for the functionality.


17-24: Approved: CreateUpgradeHandler function.

The CreateUpgradeHandler function is correctly defined to run migrations.


26-49: Approved: ConfigureFeeMarketModule function.

The ConfigureFeeMarketModule function is correctly defined to configure the fee market module.

app/upgrades/types.go (4)

Line range hint 1-1: Approved: Package declaration.

The package declaration is correct.


Line range hint 3-21: Approved: Imports.

The imports are correctly included and necessary for the functionality.


23-23: Approved: Addition of FeeMarketKeeper.

The FeeMarketKeeper field is correctly added to the AppKeepers struct.


Line range hint 24-36: Approved: Struct and interface definitions.

The struct and interface definitions are correctly defined.

app/ante/ante_test.go (3)

1-1: Approved: Package declaration.

The package declaration is correct.


3-18: Approved: Imports.

The imports are correctly included and necessary for the functionality.


20-115: Approved: Test cases for MempoolDecorator.

The test cases for MempoolDecorator are correctly defined and cover various scenarios.

app/ante/suite.go (2)

128-143: LGTM!

The function correctly creates and sets test accounts.


60-125: Verify FeeMarketKeeper initialization.

The initialization of FeeMarketKeeper appears correct, but ensure that all necessary parameters and configurations are included.

Verification successful

Verified FeeMarketKeeper initialization.

The initialization of FeeMarketKeeper in app/ante/suite.go is correctly implemented, including the use of DenomResolverImpl which properly implements the necessary interface and methods.

  • app/ante/suite.go: Initialization of FeeMarketKeeper with DenomResolverImpl is correct.
  • app/ante/ante.go: DenomResolverImpl correctly implements feemarkettypes.DenomResolver.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the initialization of FeeMarketKeeper in the codebase.

# Test: Search for the initialization of FeeMarketKeeper. Expect: Proper initialization with necessary parameters.
rg --type go $'feemarketkeeper.NewKeeper'

Length of output: 390


Script:

#!/bin/bash
# Description: Locate the definition and implementation of DenomResolverImpl in the codebase.

# Test: Search for the definition of DenomResolverImpl. Expect: Proper structure and implementation.
rg --type go 'type DenomResolverImpl'

Length of output: 88


Script:

#!/bin/bash
# Description: Extract the full definition and methods of DenomResolverImpl in the codebase.

# Test: Search for the definition and methods of DenomResolverImpl. Expect: Proper structure and implementation.
rg --type go --context 10 'type DenomResolverImpl struct' app/ante/ante.go

Length of output: 990

app/ante/ante.go (3)

27-38: LGTM!

The HandlerOptions struct correctly integrates the feemarket package by adding FeeMarketKeeper and AccountKeeper.


137-149: LGTM!

The ExtraDenoms function correctly retrieves the extra denoms registered via governance.

Tools
GitHub Check: devskim

[notice] 137-137: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment


40-91: Verify FeeMarketCheckDecorator integration.

The integration of FeeMarketCheckDecorator appears correct, but ensure it aligns with the expected behavior and all necessary parameters are included.

app/app.go (10)

55-59: Imports for FeeMarket module look good.

The new imports for the feemarket module are necessary for the integration and appear correctly added.


218-219: Module account permissions for FeeMarket module look good.

The permissions for feemarkettypes.ModuleName and feemarkettypes.FeeCollectorName are correctly specified.


260-260: Addition of FeeMarketKeeper to EveApp struct looks good.

The FeeMarketKeeper is correctly added to the EveApp struct.


610-611: Initialization of FeeMarketKeeper in NewEveApp looks good.

The FeeMarketKeeper is initialized with the correct parameters.


1002-1006: Setting denom resolver for FeeMarketKeeper looks good.

The denom resolver is set to a test variant, which is appropriate for the context.


1086-1102: Changes to setAnteHandler look good.

The FeeMarketKeeper and AccountKeeper are correctly added to HandlerOptions.


1114-1125: Changes to setPostHandler look good.

The FeeMarketKeeper is correctly added to PostHandlerOptions.


824-824: Addition of FeeMarket module to ModuleManager looks good.

The feemarket.NewAppModule is correctly added to the ModuleManager.


861-861: Changes to SetOrderBeginBlockers look good.

The feemarkettypes.ModuleName is correctly added to SetOrderBeginBlockers.


886-886: Changes to SetOrderEndBlockers look good.

The feemarkettypes.ModuleName is correctly added to SetOrderEndBlockers.

app/upgrades/v1/upgrades.go Show resolved Hide resolved
app/ante/ante.go Show resolved Hide resolved
app/ante/ante.go Outdated Show resolved Hide resolved

const (
// UpgradeName defines the on-chain upgrade name.
UpgradeName = "v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good please change it to v0.0.6, also change folder name

Copy link
Contributor

Choose a reason for hiding this comment

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

also we need add v0.0.6 upgrade to here -> https://github.com/eve-network/eve/blob/main/app/upgrades.go#L28

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think v0.1.0 is more suitable since we are adding a new module

Copy link
Contributor

Choose a reason for hiding this comment

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

let do it, thank you

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

Outside diff range and nitpick comments (1)
app/upgrades.go (1)

Line range hint 75-105:
Function correctly sets up key tables but uses deprecated methods.

The function correctly sets up the key tables but uses deprecated methods. Consider using the recommended alternatives.

- keyTable = authtypes.ParamKeyTable() //nolint:staticcheck
+ keyTable = authtypes.NewParamKeyTable()

- keyTable = banktypes.ParamKeyTable() //nolint:staticcheck
+ keyTable = banktypes.NewParamKeyTable()

- keyTable = stakingtypes.ParamKeyTable() //nolint:staticcheck
+ keyTable = stakingtypes.NewParamKeyTable()

- keyTable = minttypes.ParamKeyTable() //nolint:staticcheck
+ keyTable = minttypes.NewParamKeyTable()

- keyTable = distrtypes.ParamKeyTable() //nolint:staticcheck
+ keyTable = distrtypes.NewParamKeyTable()

- keyTable = slashingtypes.ParamKeyTable() //nolint:staticcheck
+ keyTable = slashingtypes.NewParamKeyTable()

- keyTable = govv1.ParamKeyTable() //nolint:staticcheck
+ keyTable = govv1.NewParamKeyTable()

- keyTable = crisistypes.ParamKeyTable() //nolint:staticcheck
+ keyTable = crisistypes.NewParamKeyTable()

- keyTable = v2.ParamKeyTable() //nolint:staticcheck
+ keyTable = v2.NewParamKeyTable()
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 635809c and 28d1bb0.

Files selected for processing (6)
  • app/ante/ante.go (1 hunks)
  • app/ante/ante_test.go (1 hunks)
  • app/ante/errors.go (1 hunks)
  • app/upgrades.go (2 hunks)
  • app/upgrades/v1/constants.go (1 hunks)
  • app/upgrades/v1/upgrades.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • app/ante/ante.go
  • app/ante/ante_test.go
  • app/upgrades/v1/constants.go
Additional context used
GitHub Check: devskim
app/upgrades/v1/upgrades.go

[notice] 16-16: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment

Additional comments not posted (7)
app/ante/errors.go (3)

8-15: Error definitions are clear and consistent.

The error messages are descriptive and provide clear information about the missing keepers.


17-19: Error message is clear and provides sufficient context.

The error message clearly states the issue with the provided denominations.


21-27: Error messages are clear and provide sufficient context.

The error messages clearly state the issues with the provided denomination and coin count.

app/upgrades/v1/upgrades.go (2)

17-35: Function handles errors and performs necessary migrations.

The function correctly handles errors and performs the necessary migrations for the upgrade.


38-61: Function correctly sets parameters and handles errors.

The function sets the parameters and state for the fee market module and correctly handles errors.

app/upgrades.go (2)

29-29: Upgrades list correctly includes the v1 upgrade.

The Upgrades list correctly includes the v1 upgrade.


Line range hint 31-73:
Function correctly registers handlers and handles errors.

The function correctly registers the upgrade handlers and sets the store loader for the current upgrade.

@faddat faddat merged commit 709efaa into main Jul 5, 2024
10 checks passed
@faddat faddat deleted the feat/feemarket branch July 5, 2024 07:52
@coderabbitai coderabbitai bot mentioned this pull request Oct 1, 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.

3 participants