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: addresses module #291

Merged
merged 32 commits into from
Oct 8, 2024
Merged

feat: addresses module #291

merged 32 commits into from
Oct 8, 2024

Conversation

cbrit
Copy link
Member

@cbrit cbrit commented Mar 13, 2024

  • Add a test for message handler that uses pagination
  • Self-review

Summary by CodeRabbit

  • New Features

    • Updated the protocol version to enhance functionality.
    • Introduced a "Query" service for querying address mappings.
    • Added functionality to manage address mappings between Cosmos and EVM addresses with new RPC methods.
  • Bug Fixes

    • Ensured proper setup for integration tests with the initialization of the genesis state for addresses.
  • Tests

    • Implemented a comprehensive integration test suite for address mapping functionalities, validating core operations such as creation, querying, and deletion.

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

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3143444 and cf334d5.

⛔ Files ignored due to path filters (14)
  • proto/buf.lock is excluded by !**/*.lock, !**/*.lock
  • x/addresses/types/query.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/addresses/types/tx.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/auction/types/query.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/auction/types/tx.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/axelarcork/types/query.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/axelarcork/types/tx.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/cellarfees/types/v1/query.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/cellarfees/types/v2/query.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/cork/types/v2/query.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/cork/types/v2/tx.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/incentives/types/query.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/pubsub/types/query.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/pubsub/types/tx.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
📒 Files selected for processing (3)
  • Makefile (2 hunks)
  • proto/addresses/v1/query.proto (1 hunks)
  • proto/addresses/v1/tx.proto (1 hunks)
🧰 Additional context used
🪛 buf
proto/addresses/v1/query.proto

5-5: import "addresses/v1/addresses.proto": file does not exist

(COMPILE)

proto/addresses/v1/tx.proto

4-4: import "cosmos/msg/v1/msg.proto": file does not exist

(COMPILE)

🔇 Additional comments (6)
Makefile (2)

394-396: LGTM: New integration test target added

The new e2e_addresses_test target is well-structured and consistent with other e2e test targets. It correctly uses the E2E_SKIP_CLEANUP environment variable and targets the TestAddresses test case.


214-215: LGTM: Proto version update

The update of protoVer from 0.13.1 to 0.15.1 is a good practice to stay current. However, please ensure that this change doesn't introduce any breaking changes in the project's protobuf definitions.

To verify the compatibility of the new proto version, you can run the following commands:

If these commands run without errors and don't show any unexpected changes in the generated files, it's likely that the update is compatible.

proto/addresses/v1/tx.proto (4)

8-15: LGTM: Service definitions are correctly specified

The Msg service and its RPC methods AddAddressMapping and RemoveAddressMapping are well-defined and follow best practices for gRPC services in Protocol Buffers.


17-22: LGTM: MsgAddAddressMapping message is properly structured

The MsgAddAddressMapping message includes the required fields evm_address and signer, and the signer option is correctly specified on line 18.


26-30: LGTM: MsgRemoveAddressMapping message is properly structured

The MsgRemoveAddressMapping message correctly includes the signer field, and the signer option is appropriately set on line 27.


4-4: ⚠️ Potential issue

Fix import: "cosmos/msg/v1/msg.proto" does not exist

The import statement on line 4 references "cosmos/msg/v1/msg.proto", but this file does not exist, as indicated by the static analysis tool. Please verify the correct import path or ensure that the required proto file is available.

Run the following script to verify the existence of the imported proto file:

🧰 Tools
🪛 buf

4-4: import "cosmos/msg/v1/msg.proto": file does not exist

(COMPILE)


package addresses.v1;

import "addresses/v1/addresses.proto";
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Fix Missing Import: 'addresses/v1/addresses.proto' does not exist

The file addresses/v1/addresses.proto does not exist in the repository. Please add the file or update the import path to resolve compilation errors.

🔗 Analysis chain

Fix missing import: 'addresses.proto' does not exist

The static analysis tool indicates that the imported file addresses/v1/addresses.proto does not exist. This will cause compilation errors. Please ensure that addresses.proto is added to the codebase or that the import path is corrected.

Run the following script to verify the existence of the imported file:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if 'addresses/v1/addresses.proto' exists in the repository.

# Test: Verify file existence. Expect: The file should exist.
if [ ! -f "addresses/v1/addresses.proto" ]; then
  echo "'addresses/v1/addresses.proto' does not exist."
else
  echo "'addresses/v1/addresses.proto' exists."
fi

Length of output: 154

🧰 Tools
🪛 buf

5-5: import "addresses/v1/addresses.proto": file does not exist

(COMPILE)

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

🧹 Outside diff range and nitpick comments (1)
integration_tests/setup_test.go (1)

487-491: LGTM: Addresses module genesis state initialized correctly.

The initialization of the addresses module's genesis state follows the established pattern used for other modules. It correctly uses the DefaultGenesisState() function and properly integrates the state into the overall application state.

For consistency with other module initializations in this file, consider adding a comment explaining the purpose of this initialization, similar to comments for other modules.

 // set addresses gen state
 addressesGenState := addressestypes.DefaultGenesisState()
 s.Require().NoError(cdc.UnmarshalJSON(appGenState[addressestypes.ModuleName], &addressesGenState))
+// Initialize with default genesis state
 appGenState[addressestypes.ModuleName] = cdc.MustMarshalJSON(&addressesGenState)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between cf334d5 and e4cfdbe.

📒 Files selected for processing (2)
  • app/app.go (12 hunks)
  • integration_tests/setup_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (13)
integration_tests/setup_test.go (1)

21-21: LGTM: New import for addresses module added correctly.

The new import for the addressestypes package is correctly formatted and placed appropriately among other project-specific imports.

app/app.go (12)

109-111: New imports for addresses module are correctly added.

The imports for the addresses module are properly included and aliased.


207-207: Registered addresses module in ModuleBasics.

Including addresses.AppModuleBasic{} in ModuleBasics ensures that the module is properly initialized.


226-226: Added addresses module to module account permissions.

Including addressestypes.ModuleName in maccPerms with nil permissions is appropriate and follows existing patterns.


284-284: Added AddressesKeeper to SommelierApp struct.

The addition of AddressesKeeper aligns with the pattern used for other module keepers, ensuring module interactions.


352-352: Registered Addresses module store key.

Adding addressestypes.StoreKey to the key store ensures the module's KVStore is correctly initialized.


534-537: Initialized AddressesKeeper properly.

The AddressesKeeper is correctly instantiated with appCodec, the store key, and the parameter subspace.


618-618: Added Addresses module to module manager.

Including addresses.NewAppModule(appCodec, app.AddressesKeeper) in the module manager allows the module to participate in application lifecycle events.


653-653: Verify the order of Addresses module in BeginBlockers.

Ensure that addressestypes.ModuleName is correctly placed in SetOrderBeginBlockers, considering any dependencies or required initialization order.


684-684: Verify the order of Addresses module in EndBlockers.

Confirm that addressestypes.ModuleName is appropriately ordered in SetOrderEndBlockers to ensure correct module finalization.


723-723: Verify the order of Addresses module in InitGenesis.

Check that addressestypes.ModuleName is correctly placed in SetOrderInitGenesis to guarantee proper genesis initialization.


759-759: Added Addresses module to simulation manager.

Including addresses.NewAppModule(appCodec, app.AddressesKeeper) in the simulation manager enables module simulation testing.


1011-1011: Initialized params subspace for Addresses module.

Adding paramsKeeper.Subspace(addressestypes.ModuleName) ensures the module can manage its parameters via the params keeper.

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