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(app): wire authz and feegrant modules to app #270

Merged
merged 4 commits into from
Jun 23, 2024

Conversation

zale144
Copy link
Contributor

@zale144 zale144 commented Jun 3, 2024

Description


Closes #174

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.

PR review checkboxes:

I have...

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Targeted PR against the correct branch
  • included the correct type prefix in the PR title
  • Linked to the GitHub issue with discussion and accepted design
  • Targets only one GitHub issue
  • Wrote unit and integration tests
  • Wrote relevant migration scripts if necessary
  • All CI checks have passed
  • Added relevant godoc comments
  • Updated the scripts for local run, e.g genesis_config_commands.sh if the PR changes parameters
  • Add an issue in the e2e-tests repo if necessary

SDK Checklist

  • Import/Export Genesis
  • Registered Invariants
  • Registered Events
  • Updated openapi.yaml
  • No usage of go map
  • No usage of time.Now()
  • Used fixed point arithmetic and not float arithmetic
  • Avoid panicking in Begin/End block as much as possible
  • No unexpected math Overflow
  • Used sendCoin and not SendCoins
  • Out-of-block compute is bounded
  • No serialized ID at the end of store keys
  • UInt to byte conversion should use BigEndian

Full security checklist here

----;

For Reviewer:

  • Confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • Confirmed all author checklist items have been addressed

---;

After reviewer approval:

  • In case the PR targets the main branch, PR should not be squash merge in order to keep meaningful git history.
  • In case the PR targets a release branch, PR must be rebased.

Summary by CodeRabbit

  • New Features

    • Integrated authz and feegrant modules from the Cosmos SDK, enhancing authorization and fee management capabilities.
  • Chores

    • Added a static check suppression directive in the ante handler to maintain code quality and prevent unnecessary warnings.

@zale144 zale144 requested review from danwt and mtsitrin June 3, 2024 14:03
@zale144 zale144 self-assigned this Jun 3, 2024
Copy link

coderabbitai bot commented Jun 3, 2024

Walkthrough

The changes in this update primarily focus on integrating the authz and feegrant modules from the cosmos-sdk package into the application. This involves updating imports, global variables, the App struct, and the initialization processes to accommodate these new modules. Additionally, a static check suppression directive has been added in the NewHandler function in app/ante/ante.go.

Changes

File(s) Change Summary
app/app.go Integration of authz and feegrant modules, including imports, global variables, App struct updates, and initialization adjustments.
app/ante/ante.go Added // nolint:staticcheck comment before a call to cosmosante.NewLegacyEip712SigVerificationDecorator.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant App
    participant AuthzKeeper
    participant FeeGrantKeeper
    User ->> App: Initialize Application
    App ->> AuthzKeeper: Initialize Authz Module
    App ->> FeeGrantKeeper: Initialize FeeGrant Module
    Note right of App: Modules are now part of the application
    User ->> App: Utilize Authz and FeeGrant Features
    App ->> AuthzKeeper: Process Authz Request
    App ->> FeeGrantKeeper: Process FeeGrant Request
Loading

Assessment against linked issues

Objective Addressed Explanation
Wire missing modules (### #174)

Poem

In the code where modules blend,
New powers to the app we send.
Authz and FeeGrant now take flight,
Enhancing functions, day and night.
With every line, the app grows strong,
A coder's dream, where all belong.


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.

Copy link

@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 f61c15c and 5517293.

Files selected for processing (1)
  • app/app.go (17 hunks)
Additional comments not posted (11)
app/app.go (11)

13-15: The imports for authz and related sub-packages have been correctly added.


34-34: The import for simapp is unchanged and correctly retained in the import block.


57-59: The imports for feegrant and related sub-packages have been correctly added.


Line range hint 201-212: The ModuleBasics variable has been correctly updated to include authzmodule.AppModuleBasic{} and feegrantmodule.AppModuleBasic{}. This is essential for the initialization of these modules.


231-231: The module permissions for authz have been correctly set to nil, indicating no specific permissions are required for this module.


Line range hint 290-304: The App struct has been updated to include AuthzKeeper and FeeGrantKeeper. This is crucial for the integration of these modules into the application.


415-420: The initialization of AuthzKeeper in the NewRollapp function is correct and follows the expected pattern for keeper initialization in Cosmos SDK applications.


458-458: The initialization of FeeGrantKeeper is correctly implemented. It's important to ensure that the keeper is properly configured before use in the application.


604-608: The NewAppModule calls for authz and feegrant are correctly placed and properly configured. This ensures that these modules are correctly integrated into the application's module manager.


157-158: The global variable kvstorekeys has been updated to include authzkeeper.StoreKey and feegrant.StoreKey. Ensure that these keys are unique and not duplicated elsewhere in the global scope.

Verification successful

The search results indicate that authzkeeper.StoreKey and feegrant.StoreKey are only used in app/app.go and there are no duplicates elsewhere in the codebase.

  • authzkeeper.StoreKey appears in lines related to key initialization and usage.
  • feegrant.StoreKey appears in lines related to key initialization and usage.

No duplicates were found, confirming that these store keys are unique in the global scope.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify uniqueness of store keys in global scope.

# Test: Search for duplicate store keys. Expect: No duplicates.
rg --type go $'authzkeeper.StoreKey|feegrant.StoreKey' app/app.go

Length of output: 282



Script:

#!/bin/bash
# Description: Verify uniqueness of store keys in the entire codebase.

# Test: Search for duplicate store keys across the entire codebase. Expect: No duplicates.
rg --type go 'authzkeeper.StoreKey|feegrant.StoreKey'

Length of output: 315


650-656: The order of module operations in SetOrderBeginBlockers, SetOrderEndBlockers, and SetOrderInitGenesis has been updated to include authz and feegrant. Verify that the order of module operations is correct and does not introduce any dependency issues.

Also applies to: 672-680, 700-719

Verification successful

The extracted lists for SetOrderBeginBlockers, SetOrderEndBlockers, and SetOrderInitGenesis are now complete. Let's analyze them to verify the order and ensure there are no dependency issues.

SetOrderBeginBlockers

app.mm.SetOrderBeginBlockers(beginBlockersList...)

The actual list is not shown in the output, so we need to extract beginBlockersList.

SetOrderEndBlockers

endBlockersList := []string{
	govtypes.ModuleName,
	stakingtypes.ModuleName,
	evmtypes.ModuleName,
	seqtypes.ModuleName,
	feemarkettypes.ModuleName,
	capabilitytypes.ModuleName,
	authtypes.ModuleName,
	authz.ModuleName,
	banktypes.ModuleName,
	distrtypes.ModuleName,
	vestingtypes.ModuleName,
	minttypes.ModuleName,
	erc20types.ModuleName,
	claimstypes.ModuleName,
	genutiltypes.ModuleName,
	feegrant.ModuleName,
	epochstypes.ModuleName,
	paramstypes.ModuleName,
}

SetOrderInitGenesis

initGenesisList := []string{
	capabilitytypes.ModuleName,
	authtypes.ModuleName,
	authz.ModuleName,
	banktypes.ModuleName,
	evmtypes.ModuleName,
	feemarkettypes.ModuleName,
	distrtypes.ModuleName,
	stakingtypes.ModuleName,
	seqtypes.ModuleName,
	vestingtypes.ModuleName,
	epochstypes.ModuleName,
	govtypes.ModuleName,
}

To verify the order of beginBlockersList, we need to extract its definition. Let's generate a script to find and display the beginBlockersList definition.


The previous script failed due to a syntax error in the grep command. Let's correct the script and re-run it to extract the definition of beginBlockersList.


The extracted lists for SetOrderBeginBlockers, SetOrderEndBlockers, and SetOrderInitGenesis are now complete. Let's analyze them to verify the order and ensure there are no dependency issues.

SetOrderBeginBlockers

beginBlockersList := []string{
	upgradetypes.ModuleName,
	capabilitytypes.ModuleName,
	minttypes.ModuleName,
	feemarkettypes.ModuleName,
	evmtypes.ModuleName,
	distrtypes.ModuleName,
	stakingtypes.ModuleName,
	seqtypes.ModuleName,
	vestingtypes.ModuleName,
	ibchost.ModuleName,
	ibctransfertypes.ModuleName,
	authtypes.ModuleName,
	authz.ModuleName,
	banktypes.ModuleName,
	govtypes.ModuleName,
	erc20types.ModuleName,
	claimstypes.ModuleName,
	genutiltypes.ModuleName,
	feegrant.ModuleName,
	epochstypes.ModuleName,
}

SetOrderEndBlockers

endBlockersList := []string{
	govtypes.ModuleName,
	stakingtypes.ModuleName,
	evmtypes.ModuleName,
	seqtypes.ModuleName,
	feemarkettypes.ModuleName,
	capabilitytypes.ModuleName,
	authtypes.ModuleName,
	authz.ModuleName,
	banktypes.ModuleName,
	distrtypes.ModuleName,
	vestingtypes.ModuleName,
	minttypes.ModuleName,
	erc20types.ModuleName,
	claimstypes.ModuleName,
	genutiltypes.ModuleName,
	feegrant.ModuleName,
	epochstypes.ModuleName,
	paramstypes.ModuleName,
}

SetOrderInitGenesis

initGenesisList := []string{
	capabilitytypes.ModuleName,
	authtypes.ModuleName,
	authz.ModuleName,
	banktypes.ModuleName,
	evmtypes.ModuleName,
	feemarkettypes.ModuleName,
	distrtypes.ModuleName,
	stakingtypes.ModuleName,
	seqtypes.ModuleName,
	vestingtypes.ModuleName,
	epochstypes.ModuleName,
	govtypes.ModuleName,
}

The modules authz and feegrant have been included in the lists. The order appears to be consistent with the dependencies mentioned in the comments, ensuring that modules are initialized in the correct sequence.

  • authz is placed after authtypes in all lists, which is logical since authz might depend on authtypes.
  • feegrant is included towards the end of the lists, which should not introduce any dependency issues.

Conclusion

The order of module operations in SetOrderBeginBlockers, SetOrderEndBlockers, and SetOrderInitGenesis appears to be correct and does not introduce any dependency issues.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the order of module operations.

# Test: Check module dependencies and order. Expect: Correct order without dependency issues.
cat app/app.go | grep -A 10 "SetOrderBeginBlockers"
cat app/app.go | grep -A 10 "SetOrderEndBlockers"
cat app/app.go | grep -A 10 "SetOrderInitGenesis"

Length of output: 1524



Script:

#!/bin/bash
# Extract the complete lists for SetOrderBeginBlockers, SetOrderEndBlockers, and SetOrderInitGenesis

# Extract the complete list for SetOrderBeginBlockers
echo "SetOrderBeginBlockers:"
cat app/app.go | grep -A 20 "SetOrderBeginBlockers"

# Extract the complete list for SetOrderEndBlockers
echo "SetOrderEndBlockers:"
cat app/app.go | grep -A 20 "SetOrderEndBlockers"

# Extract the complete list for SetOrderInitGenesis
echo "SetOrderInitGenesis:"
cat app/app.go | grep -A 20 "SetOrderInitGenesis"

Length of output: 2709



Script:

#!/bin/bash
# Extract the definition of beginBlockersList

# Search for the definition of beginBlockersList
echo "beginBlockersList definition:"
cat app/app.go | grep -A 20 "beginBlockersList := []string"

Length of output: 167



Script:

#!/bin/bash
# Extract the definition of beginBlockersList

# Search for the definition of beginBlockersList
echo "beginBlockersList definition:"
cat app/app.go | grep -A 20 "beginBlockersList := \[\]string"

Length of output: 670

@omritoptix omritoptix changed the title feat(app) Wire authz and feegrant modules to app feat(app): wire authz and feegrant modules to app Jun 3, 2024
Copy link

@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 5517293 and ba9e8e7.

Files selected for processing (1)
  • app/ante/ante.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • app/ante/ante.go

@mtsitrin mtsitrin merged commit dad400c into main Jun 23, 2024
54 of 56 checks passed
@mtsitrin mtsitrin deleted the zale144/174-wire-missing-modules branch June 23, 2024 06:44
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.

Wire missing modules
3 participants