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

chore: reorganize repo structure #19

Merged
merged 1 commit into from
Oct 30, 2024
Merged

chore: reorganize repo structure #19

merged 1 commit into from
Oct 30, 2024

Conversation

johnletey
Copy link
Member

@johnletey johnletey commented Oct 30, 2024

Summary by CodeRabbit

  • New Features

    • Streamlined import paths for the forwarding module, enhancing organization and accessibility.
    • Integration of forwarding functionality into the transfer module via middleware.
  • Bug Fixes

    • Adjusted handling of account registration and clearing processes to ensure consistency with new type references.
  • Documentation

    • Updated comments for clarity regarding module structure and functionality.
  • Chores

    • Refactored import paths across multiple files to simplify the codebase and improve maintainability.

@johnletey johnletey added this to the v2.0.0 milestone Oct 30, 2024
@johnletey johnletey requested a review from g-luca October 30, 2024 11:38
@johnletey johnletey self-assigned this Oct 30, 2024
Copy link

coderabbitai bot commented Oct 30, 2024

Walkthrough

The changes in this pull request involve significant restructuring of import paths across multiple files, primarily within the forwarding module. Key adjustments include updating paths from github.com/noble-assets/forwarding/v2/x/forwarding/... to github.com/noble-assets/forwarding/v2/..., reflecting a more streamlined directory organization. Additionally, modifications to the Makefile enhance commands related to building and testing, while various files see updates to type references, particularly in relation to the types package. Overall, the functionality remains intact despite these organizational changes.

Changes

File Change Summary
Makefile Updated commands related to building, formatting, linting, and testing; changed path in local-image target.
ante.go Updated import path for types package.
api/module/v1/module.pulsar.go Updated authority field metadata and adjusted size calculation in protobuf descriptor.
client/cli/query.go Updated import path for types package.
client/cli/tx.go Updated import path for types package.
e2e/forwarding_test.go Updated import path for forwardingtypes package.
genesis.go Updated import paths for keeper and types packages.
keeper/keeper.go Updated import path for forwarding/types; adjusted PendingForwards field initialization.
keeper/migrations.go Updated import path for v1 package.
keeper/msg_server.go Updated import path for forwarding types in various methods.
keeper/query_server.go Updated import path for types package.
keeper/state.go Updated import path for types; specified return type for GetPendingForwards.
middleware.go Updated import paths for keeper and types packages.
migrations/v1/state.go Updated import path for types package.
module.go Simplified import paths and updated comments for clarity.
proto/generate.sh Added commands to copy and remove directories for file management.
proto/noble/forwarding/module/v1/module.proto Updated go_import option for Module message.
proto/noble/forwarding/v1/account.proto Updated go_package option.
proto/noble/forwarding/v1/events.proto Updated go_package option.
proto/noble/forwarding/v1/genesis.proto Updated go_package option.
proto/noble/forwarding/v1/packet.proto Updated go_package option.
proto/noble/forwarding/v1/query.proto Updated go_package option.
proto/noble/forwarding/v1/tx.proto Updated go_package option.
simapp/ante.go Updated import paths for forwarding and types packages.
simapp/app.go Updated import paths for forwarding and keeper modules.
simapp/legacy.go Updated import path for forwarding module; integrated middleware into transfer stack.

Possibly related PRs

Suggested reviewers

  • g-luca

Poem

🐇 In the code where paths align,
A rabbit hops, and all is fine.
Imports trimmed, the structure neat,
With every change, we feel the beat.
Forwarding flows, like streams that twine,
Hooray for changes, oh so 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>, please review it.
    • 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @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 using 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.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

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

@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 (4)
proto/generate.sh (2)

7-7: Add safety checks to prevent accidental overwrites.

While the copy operation aligns with the repository restructuring, consider adding checks to prevent accidental overwrites of existing files in the api/ directory.

Consider this safer approach:

-cp -r api/noble/forwarding/* api/
+# Ensure we don't accidentally overwrite existing files
+if [ -d "api/noble/forwarding" ]; then
+  if [ -n "$(ls -A api/noble/forwarding)" ]; then
+    if [ -n "$(ls -A api/)" ]; then
+      echo "Warning: api/ directory is not empty. Please verify before proceeding."
+      read -p "Continue? (y/N) " confirm
+      if [ "$confirm" != "y" ]; then
+        exit 1
+      fi
+    fi
+    cp -r api/noble/forwarding/* api/
+  else
+    echo "Error: api/noble/forwarding is empty"
+    exit 1
+  fi
+else
+  echo "Error: api/noble/forwarding directory not found"
+  exit 1
+fi

7-7: Add comments to explain the restructuring operations.

The file operations are correct and align with the repository restructuring goals. However, adding comments would improve maintainability.

Consider adding these comments:

+# Copy forwarding API files to the root api/ directory as part of repository flattening
 cp -r api/noble/forwarding/* api/

+# Clean up the old directory structure
 rm -rf api/noble

Also applies to: 10-10

migrations/v1/state.go (1)

Line range hint 29-45: Consider handling ParseUint errors in GetAllNumOfForwards.

Similar to GetAllNumOfAccounts, this function also silently ignores potential parsing errors.

Consider the same error handling improvement:

-		count, _ := strconv.ParseUint(string(iterator.Value()), 10, 64)
+		count, err := strconv.ParseUint(string(iterator.Value()), 10, 64)
+		if err != nil {
+			// Consider logging the error with the problematic channel
+			continue
+		}
ante.go (1)

Line range hint 41-58: Document the control flow in AnteHandle.

The function has complex branching logic with multiple early returns to the underlying handler. Consider adding documentation to explain:

  1. Why we only process single-message transactions
  2. The significance of the balance check
  3. The relationship between the signer and generated address

Add comments explaining the logic:

 func (d SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
+    // Only process single-message transactions as forwarding accounts are designed for single operations
     if msgs := tx.GetMsgs(); len(msgs) == 1 {
         msg, ok := msgs[0].(*types.MsgRegisterAccount)
         if !ok {
             return d.underlying.AnteHandle(ctx, tx, simulate, next)
         }
 
+        // Generate the expected address and verify the account has a balance
         address := types.GenerateAddress(msg.Channel, msg.Recipient, msg.Fallback)
         balance := d.bank.GetAllBalances(ctx, address)
 
+        // Ensure the account is funded and the signer matches the generated address
         if balance.IsZero() || msg.Signer != address.String() {
             return d.underlying.AnteHandle(ctx, tx, simulate, next)
         }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 93780a9 and 0bc6822.

⛔ Files ignored due to path filters (9)
  • api/v1/query_grpc.pb.go is excluded by !**/*.pb.go
  • api/v1/tx_grpc.pb.go is excluded by !**/*.pb.go
  • types/account.pb.go is excluded by !**/*.pb.go
  • types/events.pb.go is excluded by !**/*.pb.go
  • types/genesis.pb.go is excluded by !**/*.pb.go
  • types/packet.pb.go is excluded by !**/*.pb.go
  • types/query.pb.go is excluded by !**/*.pb.go
  • types/query.pb.gw.go is excluded by !**/*.pb.gw.go
  • types/tx.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (26)
  • Makefile (2 hunks)
  • ante.go (1 hunks)
  • api/module/v1/module.pulsar.go (1 hunks)
  • client/cli/query.go (1 hunks)
  • client/cli/tx.go (1 hunks)
  • e2e/forwarding_test.go (1 hunks)
  • genesis.go (1 hunks)
  • keeper/keeper.go (1 hunks)
  • keeper/migrations.go (1 hunks)
  • keeper/msg_server.go (1 hunks)
  • keeper/query_server.go (1 hunks)
  • keeper/state.go (1 hunks)
  • middleware.go (1 hunks)
  • migrations/v1/state.go (1 hunks)
  • module.go (3 hunks)
  • proto/generate.sh (1 hunks)
  • proto/noble/forwarding/module/v1/module.proto (1 hunks)
  • proto/noble/forwarding/v1/account.proto (1 hunks)
  • proto/noble/forwarding/v1/events.proto (1 hunks)
  • proto/noble/forwarding/v1/genesis.proto (1 hunks)
  • proto/noble/forwarding/v1/packet.proto (1 hunks)
  • proto/noble/forwarding/v1/query.proto (1 hunks)
  • proto/noble/forwarding/v1/tx.proto (1 hunks)
  • simapp/ante.go (1 hunks)
  • simapp/app.go (2 hunks)
  • simapp/legacy.go (1 hunks)
✅ Files skipped from review due to trivial changes (9)
  • e2e/forwarding_test.go
  • genesis.go
  • middleware.go
  • proto/noble/forwarding/v1/account.proto
  • proto/noble/forwarding/v1/genesis.proto
  • proto/noble/forwarding/v1/packet.proto
  • proto/noble/forwarding/v1/query.proto
  • proto/noble/forwarding/v1/tx.proto
  • simapp/ante.go
🔇 Additional comments (27)
proto/noble/forwarding/module/v1/module.proto (1)

9-9: LGTM! Verify import path consistency across generated code.

The update to the go_import option aligns with the repository restructuring objective, simplifying the import path from github.com/noble-assets/forwarding/v2/x/forwarding to github.com/noble-assets/forwarding/v2.

Let's verify the consistency of this change across the codebase:

✅ Verification successful

This will help us verify if the generated files are using the correct import paths after the restructuring.


This will help us verify the module path in go.mod and check other proto files for consistency with the new import path.


Import path update is consistent across the codebase

The verification shows that:

  1. The module path in go.mod is correctly set to github.com/noble-assets/forwarding/v2
  2. All proto files consistently use the new import path structure
  3. No references to the old path pattern github.com/noble-assets/forwarding/v2/x/forwarding were found
  4. Generated .pb.go files exist and don't contain any references to the old import path
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to the old import path pattern
# to ensure the restructuring is complete and consistent.

echo "Checking for old import path references..."
rg "github.com/noble-assets/forwarding/v2/x/forwarding" --type go

echo "Checking for generated proto files that might need updating..."
fd -e pb.go

Length of output: 465


Script:

#!/bin/bash
# Let's check the import statements in the generated pb.go files
echo "Checking imports in generated pb.go files..."
for file in $(fd -e pb.go); do
    echo "=== $file ==="
    rg "^import|noble-assets/forwarding" "$file"
done

Length of output: 1397


Script:

#!/bin/bash
# Let's check the go.mod file to verify the module path
echo "Checking module path in go.mod..."
cat go.mod

# Also check for any references to forwarding in proto files
echo -e "\nChecking proto files for forwarding references..."
fd -e proto | xargs rg "noble-assets/forwarding"

Length of output: 16201

proto/noble/forwarding/v1/events.proto (2)

5-5: Note: Breaking change in import paths.

This change in the go_package path is a breaking change that will affect any external consumers importing the generated Go types. Ensure this change is documented in the changelog and that a major version bump is planned if this package is published externally.

Let's check if there's a changelog or version file that needs updating:

#!/bin/bash
# Description: Check for changelog files that might need updating

# Test: Look for common changelog files
# Expect: Files that might need version/breaking change documentation
fd -i "changelog|version" --type f

5-5: LGTM! Verify consistency across proto files.

The simplified import path aligns with the repository restructuring objective.

Let's verify that this change is consistently applied across all proto files:

✅ Verification successful

✅ Import paths are consistently updated across all proto files

All proto files in the repository use the new simplified import path github.com/noble-assets/forwarding/v2/types, confirming that the change has been consistently applied throughout the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if all proto files use the new import path consistently

# Test: Search for any proto files still using the old import path
# Expect: No results, as all should be updated
rg -t proto "github.com/noble-assets/forwarding/v2/x/forwarding/types"

# Test: Verify all proto files use the new import path
# Expect: All proto files should show the new path
rg -t proto "github.com/noble-assets/forwarding/v2/types"

Length of output: 144


Script:

#!/bin/bash
# Let's try a different approach to verify proto files and their go_package options

# First, find all proto files
fd -e proto | while read -r file; do
  echo "=== $file ==="
  grep "go_package" "$file"
done

Length of output: 1667

keeper/migrations.go (1)

6-6: LGTM! Import path update aligns with repository restructuring.

The simplified import path is consistent with the PR's objective of reorganizing the repository structure.

Let's verify the migration package exists in the new location:

✅ Verification successful

Import path update verified and confirmed working

The verification shows that:

  • The new migrations/v1 directory exists with state.go
  • Both required functions GetAllNumOfAccounts and GetAllNumOfForwards are present in the new location
  • No references to the old import path remain
  • The new import path is correctly used in keeper/migrations.go
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the v1 migrations package exists in the new location
# and contains the required functions.

# Test 1: Check if the migrations/v1 directory exists
fd -t d "migrations/v1$"

# Test 2: Verify the required functions exist in the new location
ast-grep --pattern 'func GetAllNumOfAccounts' 
ast-grep --pattern 'func GetAllNumOfForwards'

Length of output: 122


Script:

#!/bin/bash
# Let's try a different approach to verify the migration functions

# Search for any files in migrations/v1 directory
fd -t f . "migrations/v1"

# Search for the functions using ripgrep for better context
rg "func GetAllNumOf(Accounts|Forwards)" -A 2

# Check if there are any Go files importing the old path
rg "github.com/noble-assets/forwarding/v2/x/forwarding/migrations/v1"

# Check if there are any Go files importing the new path
rg "github.com/noble-assets/forwarding/v2/migrations/v1"

Length of output: 661

migrations/v1/state.go (2)

8-8: LGTM! Import path update aligns with repository restructuring.

The simplified import path is consistent with the PR's objective of reorganizing the repository structure.


Line range hint 11-27: Consider handling ParseUint errors in GetAllNumOfAccounts.

The function silently ignores potential parsing errors from strconv.ParseUint. While this might be intentional for migration purposes, it's worth considering whether invalid data should be logged or handled differently.

Let's check if there are any error handling patterns in other migration files:

Consider adding error logging:

-		count, _ := strconv.ParseUint(string(iterator.Value()), 10, 64)
+		count, err := strconv.ParseUint(string(iterator.Value()), 10, 64)
+		if err != nil {
+			// Consider logging the error with the problematic channel
+			continue
+		}
client/cli/query.go (1)

10-10: LGTM! Import path update aligns with repository restructuring.

The simplified import path is consistent with the PR's objective of reorganizing the repository structure.

Let's verify the consistency of import path changes across the codebase:

✅ Verification successful

Import path changes are consistently applied across the codebase

The verification results show:

  • No instances of the old import path github.com/noble-assets/forwarding/v2/x/forwarding/types remain
  • The new import path github.com/noble-assets/forwarding/v2/types is consistently used across all relevant files including:
    • Go source files (*.go)
    • Protocol buffer definitions with correct go_package options
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all imports of the types package use the new path structure

# Test 1: Check for any remaining old import paths
echo "Checking for old import paths..."
rg "github.com/noble-assets/forwarding/v2/x/forwarding/types"

# Test 2: Verify new import paths are used consistently
echo "Checking new import paths..."
rg "github.com/noble-assets/forwarding/v2/types"

Length of output: 1746

ante.go (3)

Line range hint 15-23: Consider documenting the gas consumption policy for forwarding accounts.

The function returns nil (no gas consumption) for forwarding account signatures. While this might be intentional, it could potentially be exploited if not properly constrained elsewhere in the system.

Let's check if there are any additional security measures in place:

#!/bin/bash
# Description: Look for additional security constraints on forwarding accounts

echo "Checking for ForwardingPubKey usage and validation..."
ast-grep --pattern 'ForwardingPubKey {
  $$$
}'

echo "Checking for related security measures..."
rg -A 5 "ForwardingPubKey"

Consider adding a comment explaining why forwarding accounts don't consume gas for signature verification.


Line range hint 47-50: Verify the balance check behavior.

The code skips custom handling when the account balance is zero. This seems to imply that forwarding accounts must be pre-funded to use custom signature verification.

Let's check the broader context of this requirement:

#!/bin/bash
# Description: Look for documentation or tests related to the balance requirement

echo "Checking for related documentation..."
rg -A 5 "balance.*zero|zero.*balance"

echo "Checking for test cases..."
fd -e go -e md -E vendor | xargs rg "TestRegisterAccount|test.*balance"

10-10: Verify consistent import path updates across the codebase.

The import path change from x/forwarding/types to types aligns with the PR's objective of reorganizing the repo structure.

Let's verify that all imports of the types package have been updated consistently:

✅ Verification successful

This script will:

  1. Search for any files still using the old path pattern
  2. Search for files using the new import path
  3. Examine the specific import block in ante.go for context

Import path changes are consistent across the codebase

The verification shows that:

  • No files are using the old import path (x/forwarding/types)
  • The new import path (github.com/noble-assets/forwarding/v2/types) is consistently used across multiple files in the codebase
  • The changes align with the repository reorganization objective
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining old import paths and verify new import paths
# are used consistently across the codebase

echo "Checking for old import paths..."
rg "github.com/noble-assets/forwarding/v2/x/forwarding/types"

echo "Verifying new import paths..."
rg "github.com/noble-assets/forwarding/v2/types"

Length of output: 355


Script:

#!/bin/bash
# Let's try a different approach using more specific search patterns

echo "Checking for any remaining old import paths..."
rg -l "x/forwarding/types"

echo "Checking current import paths..."
rg -l "\"github.com/noble-assets/forwarding/v2/types\""

echo "Checking import statements in ante.go..."
cat ante.go | grep -A 5 -B 5 "import"

Length of output: 1080

client/cli/tx.go (1)

10-10: LGTM! Verify consistent import path updates across the codebase.

The import path change aligns with the PR's objective of reorganizing the repo structure. The modification from github.com/noble-assets/forwarding/v2/x/forwarding/types to github.com/noble-assets/forwarding/v2/types reflects a more streamlined directory organization.

Let's verify that this import path change has been consistently applied across the codebase:

✅ Verification successful

Import path update has been consistently applied across the codebase

The verification confirms that:

  • No instances of the old import path github.com/noble-assets/forwarding/v2/x/forwarding/types remain in the codebase
  • The new import path github.com/noble-assets/forwarding/v2/types is consistently used across all relevant files including:
    • Proto definitions
    • Core module files
    • Keeper implementations
    • CLI commands
    • Tests
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining old import paths and confirm new import paths
# are used consistently

echo "Checking for any remaining old import paths..."
rg "github\.com/noble-assets/forwarding/v2/x/forwarding/types"

echo "Verifying new import paths..."
rg "github\.com/noble-assets/forwarding/v2/types"

Length of output: 1782

keeper/query_server.go (2)

10-10: LGTM: Import path update aligns with repo restructuring.

The change simplifies the import path by moving types to a more accessible location, which is consistent with the PR's objective.


10-10: Verify consistent usage of the new types import path.

Let's ensure all files have been updated to use the new import path consistently.

✅ Verification successful

Import path migration is complete and consistent

The verification shows that:

  • No references to the old import path github.com/noble-assets/forwarding/v2/x/forwarding/types remain
  • The new import path github.com/noble-assets/forwarding/v2/types is consistently used across all relevant files
  • The types directory exists at the repository root, confirming the correctness of the new import path
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to the old import path
# and verify that new import path exists and is used consistently

echo "Checking for any remaining old import paths..."
rg "github.com/noble-assets/forwarding/v2/x/forwarding/types"

echo "Verifying usage of new import path..."
rg "github.com/noble-assets/forwarding/v2/types"

echo "Checking if types package exists at new location..."
fd "^types$" --type d

Length of output: 1936

Makefile (2)

14-14: LGTM: Section header update is more descriptive.

The change from "Formatting & Linting" to "Tooling" better represents the section's purpose and allows for future expansion of development tools.


63-63: Verify the chains.yaml file location.

Moving the configuration file to the e2e directory aligns with the repository reorganization objective. However, let's verify the file exists in the new location.

✅ Verification successful

Configuration file location verified successfully

The chains.yaml file exists in the expected e2e directory, confirming the successful repository reorganization for this configuration file.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify chains.yaml exists in the e2e directory
# Expected: File should exist in e2e directory

if [ -f "e2e/chains.yaml" ]; then
    echo "✅ chains.yaml found in e2e directory"
else
    echo "❌ chains.yaml not found in e2e directory"
    # Check if it still exists in the old location
    if [ -f "chains.yaml" ]; then
        echo "⚠️ chains.yaml found in root directory - needs to be moved"
    fi
fi

Length of output: 111

keeper/state.go (1)

7-7: LGTM: Import path update aligns with repository restructuring.

The simplified import path github.com/noble-assets/forwarding/v2/types reflects the more streamlined directory organization, making the types package more accessible while maintaining the same functionality.

Let's verify the consistency of this import path change across the codebase:

✅ Verification successful

Import path migration successfully verified across the codebase

The verification confirms that:

  • No references to the old import path github.com/noble-assets/forwarding/v2/x/forwarding/types remain in the codebase
  • The new import path github.com/noble-assets/forwarding/v2/types is consistently used across all relevant files, including:
    • Go source files (*.go)
    • Protocol buffer definitions (*.proto)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all imports of the types package use the new path
# and that there are no remaining references to the old path

# Check for any remaining references to the old import path
echo "Checking for old import paths..."
rg "github.com/noble-assets/forwarding/v2/x/forwarding/types"

# Check for consistency of new import path
echo "Verifying new import paths..."
rg "github.com/noble-assets/forwarding/v2/types"

Length of output: 1748

simapp/legacy.go (2)

18-18: LGTM: Import path update aligns with repository restructuring.

The simplified import path follows Go module best practices for a cleaner repository structure.


Line range hint 69-70: Verify middleware integration across the codebase.

The middleware integration looks correct, but let's verify that the forwarding middleware is consistently integrated across other IBC-related configurations in the codebase.

✅ Verification successful

Middleware integration is correctly implemented and isolated

The verification shows that:

  • The forwarding middleware is properly integrated in a single location (simapp/legacy.go)
  • The middleware implementation follows the standard IBC middleware pattern with all required keeper dependencies
  • The IBC router configuration is correctly set up with the transfer module and middleware stack
  • The necessary keeper references are properly set after the middleware setup
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent forwarding middleware integration patterns
# Test 1: Check for other instances of forwarding middleware integration
rg -A 3 "NewMiddleware.*ForwardingKeeper" 

# Test 2: Verify IBC module initialization patterns
rg -A 5 "NewIBCModule.*transfer"

Length of output: 367


Script:

#!/bin/bash
# Let's try a broader search to understand the IBC module setup patterns
# Check for IBC router configuration patterns
rg -A 5 "NewRouter\(\).AddRoute"

# Check for transfer module initialization patterns
rg -A 5 "transfer.*Module.*New"

# Look for any other middleware usage patterns
rg -A 5 "NewMiddleware"

# Search for transfer stack initialization
rg -A 5 "transferStack.*:="

Length of output: 1503

simapp/app.go (2)

47-47: LGTM! Verify keeper import usage.

The keeper import path update is consistent with the module reorganization.

Let's verify the keeper import path changes:

✅ Verification successful

Import path change verified successfully

The verification confirms:

  • No instances of the old keeper import path remain in the codebase
  • The keeper type is correctly referenced in simapp/app.go using the new import alias
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining old keeper import paths and verify new usage
# Test: Search for old keeper import paths. Expect: No results
rg "github.com/noble-assets/forwarding/v2/x/forwarding/keeper"

# Test: Verify keeper type usage remains correct. Expect: Consistent type references
ast-grep --pattern 'forwardingkeeper.Keeper'

Length of output: 167


30-30: LGTM! Verify import path consistency across the codebase.

The simplified import path aligns with the repository reorganization objective.

Let's verify the consistency of the new import path across the codebase:

✅ Verification successful

Import path changes are correctly implemented

The verification shows that:

  • The old import path github.com/noble-assets/forwarding/v2/x/forwarding is not used anywhere in the codebase
  • The new import structure is consistently used across all relevant files
  • The keeper is correctly imported using forwardingkeeper "github.com/noble-assets/forwarding/v2/keeper"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining old import paths that might have been missed
# Test: Search for old import paths. Expect: No results
rg "github.com/noble-assets/forwarding/v2/x/forwarding[^/]"

# Test: Verify new import path usage. Expect: Consistent usage across files
rg "github.com/noble-assets/forwarding/v2\""

Length of output: 167


Script:

#!/bin/bash
# Let's check the actual import statements and keeper usage
rg -A 2 "forwardingkeeper" simapp/app.go

# Also check if there are any other files using the old import path pattern
rg -l "github.com/noble-assets/forwarding/v2/x/forwarding"

# And verify the new import path usage
rg -l "github.com/noble-assets/forwarding/v2"

Length of output: 957

keeper/msg_server.go (1)

13-13: LGTM: Import path update aligns with repository restructuring.

The simplified import path is part of the repository reorganization, removing the redundant x/forwarding segment. The change is purely structural and doesn't affect functionality.

keeper/keeper.go (1)

18-18: LGTM! Import path change aligns with repository restructuring.

The simplified import path is consistent with the PR objective of reorganizing the repository structure.

Let's verify that this import path change is consistently applied across the codebase:

✅ Verification successful

Import path change is consistently applied across the codebase

The verification results show that:

  • No instances of the old import path github.com/noble-assets/forwarding/v2/x/forwarding/types remain
  • The new import path github.com/noble-assets/forwarding/v2/types is consistently used across all relevant files including:
    • Go source files (*.go)
    • Proto files with correct go_package option
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that old import paths are fully replaced with new ones
# Test: Search for any remaining old import paths. Expect: No results
rg "github.com/noble-assets/forwarding/v2/x/forwarding/types"

# Test: Verify new import path usage. Expect: Multiple results showing consistent usage
rg "github.com/noble-assets/forwarding/v2/types"

Length of output: 1604

module.go (4)

24-28: LGTM: Import paths simplified consistently

The import path changes align well with the repository restructuring goal, maintaining a cleaner and more consistent structure.


32-32: LGTM: Documentation updated for clarity

The comment update appropriately reflects the module's new organization while maintaining clarity.


125-125: LGTM: Error messages updated consistently

The panic messages have been updated to reflect the new module organization while maintaining their clarity and informativeness.

Also applies to: 245-245


24-28: Verify consistency of restructuring changes

Let's verify that similar changes have been applied consistently across the codebase.

Also applies to: 32-32, 125-125, 245-245

✅ Verification successful

Import restructuring is consistent across the codebase

The occurrences of "x/forwarding" found in the codebase are all in documentation files (spec/*.md) and comments referencing historical code (migrations/v1/state.go), which is expected and correct. These references are part of documentation and migration code comments that intentionally reference the old paths.

No instances of the old import path "github.com/noble-assets/forwarding/v2/x/forwarding" were found in the actual code, confirming that the import restructuring has been applied consistently.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of old import paths or "x/forwarding" references

# Check for any remaining old import paths
echo "Checking for old import paths..."
rg "github\.com/noble-assets/forwarding/v2/x/forwarding" -l

# Check for remaining "x/forwarding" references in comments and strings
echo "Checking for old x/forwarding references..."
rg "x/forwarding" -l

# The absence of results would confirm consistent updates

Length of output: 379


Script:

#!/bin/bash
# Let's check the actual content of these files to see if they contain outdated references
# that need updating or if they're just documentation/historical references

echo "=== Checking content of files with x/forwarding references ==="

for file in migrations/v1/state.go spec/05-cli.md spec/04-queries.md spec/03-events.md spec/02-messages.md spec/01-state.md CHANGELOG.md; do
    echo -e "\n=== Content of $file containing 'x/forwarding' ==="
    rg "x/forwarding" "$file" -C 2
done

Length of output: 4958

api/module/v1/module.pulsar.go (1)

495-517: LGTM! Import path updates are consistent with repository restructuring.

The changes to the protobuf descriptor reflect the repository reorganization from github.com/noble-assets/forwarding/v2/x/forwarding/... to github.com/noble-assets/forwarding/v2/.... This aligns with the PR objectives.

Let's verify the consistency of import paths across the codebase:

✅ Verification successful

Import path updates are consistent with repository restructuring

The verification confirms:

  • No instances of the old path github.com/noble-assets/forwarding/v2/x/forwarding were found
  • The new path structure github.com/noble-assets/forwarding/v2/api is properly adopted in module.go and proto/buf.gen.pulsar.yaml
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining old import paths
rg -l "github\.com/noble-assets/forwarding/v2/x/forwarding" 

# Check adoption of new import paths
rg -l "github\.com/noble-assets/forwarding/v2/(?!x/forwarding)"

Length of output: 425


Script:

#!/bin/bash
# Check for any remaining old import paths
rg -l "github\.com/noble-assets/forwarding/v2/x/forwarding"

# Check new import paths more reliably
rg -l "github\.com/noble-assets/forwarding/v2/api"
rg -l "github\.com/noble-assets/forwarding/v2/pkg"

Length of output: 203

@johnletey johnletey merged commit 4f8f569 into main Oct 30, 2024
11 checks passed
@johnletey johnletey deleted the reorganize branch October 30, 2024 22:06
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