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: upgrade + audits fixes #87

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat: upgrade + audits fixes #87

wants to merge 9 commits into from

Conversation

beer-1
Copy link
Member

@beer-1 beer-1 commented Feb 25, 2025

Description

This PR includes dependency upgrades and also includes audit fixes.
need to wait: initia-labs/initia#349


Author Checklist

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.

I have...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Integrated enhanced blockchain and mempool functionalities, including new API support and service registration.
    • Introduced an additional configuration parameter to offer more flexible transaction gas management.
  • Documentation

    • Expanded API documentation with new endpoints detailing auction parameters and transaction distribution.
    • Updated configuration files to include new SDK documentation URLs.
  • Chores/Updates

    • Updated environment settings, upgrade routines, and dependency versions to ensure improved consistency and performance.
    • Added support for a new dependency, block-sdk, in the Swagger generation process.
    • Removed pull request trigger from the GitHub Actions workflow for streamlined operations.

@beer-1 beer-1 self-assigned this Feb 25, 2025
@beer-1 beer-1 requested a review from a team as a code owner February 25, 2025 05:44
Copy link

coderabbitai bot commented Feb 25, 2025

Walkthrough

This pull request updates various components across the project. It bumps the libmovevm version in the Dockerfile and integrates Block SDK functionalities into the application by registering new API routes and services. A new genesis parameter is added, and the keeper initialization is simplified by removing the community pool keeper. The upgrade process is modified to run migrations, while API documentation expands with auction and mempool endpoints. Dependency versions in go.mod are updated, and the protoc-swagger generation script now includes block-sdk integration.

Changes

File(s) Change Summary
Dockerfile Updated LIBMOVEVM_VERSION from v0.6.0 to v0.7.0.
app/app.go, app/genesis.go, app/keepers/keepers.go, app/upgrade.go Integrated new Block SDK functionality: added API route and mempool service registration (with type assertion), added HookMaxGas parameter, removed community pool keeper, and updated upgrade control flow with migration handling.
client/docs/config.json, client/docs/swagger-ui/swagger.yaml Added new API endpoints and definitions for auction and mempool modules with corresponding Swagger documentation.
go.mod Updated multiple dependency versions and added a replace directive for github.com/cosmos/iavl.
scripts/protoc-swagger-gen.sh Introduced new block-sdk variables, clone logic, and updated proto directories to include block-sdk files in the Swagger generation process.
.github/workflows/docker.yml Removed pull_request trigger section from GitHub Actions workflow configuration.

Possibly related PRs

Suggested reviewers

  • Vritra4
  • sh-cha

Poem

I'm a hopping rabbit in a code burrow bright,
Skipping through Dockerfiles and routes at night.
With mempool magic and docs so new,
Dependencies updated—all for you.
My whiskers twitch with a joyful cheer,
Celebrating code blossoms as the upgrade draws near!
🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 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 generate docstrings to generate docstrings for this 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

🧹 Nitpick comments (2)
client/docs/swagger-ui/swagger.yaml (1)

73436-73571: Review of Schema Definitions for SDK Auction and Mempool

The new schema definitions (sdk.auction.v1.Params, sdk.auction.v1.QueryParamsResponse, and sdk.mempool.v1.GetTxDistributionResponse) are clearly articulated. The properties, types, and descriptions are well detailed, making it clear to API consumers what to expect.

  • For sdk.auction.v1.Params and sdk.auction.v1.QueryParamsResponse, ensure that property values (e.g., max_bundle_size, escrow_account_address, etc.) match the underlying data model and that any required validations are documented.
  • Consider normalizing the YAML scalar style (e.g., use of >- versus |-) throughout the definitions for improved readability.
  • Confirm that the structure of QueryParamsResponse, which includes both the nested params object and the standalone escrow_address_string property, precisely reflects the API response from the backend.
go.mod (1)

288-291: Temporary Replace Directive for Cosmos IAVL
A temporary replace directive has been added to substitute github.com/cosmos/iavl with github.com/initia-labs/iavl (v0.0.0-20250223141407-caf697dd4712) to address a statesync problem (see cosmos/cosmos-sdk#23740). Consider adding a TODO comment for future re-evaluation so that this temporary fix can be removed once the upstream issue is resolved.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f4c8d68 and c6b7fec.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (9)
  • Dockerfile (1 hunks)
  • app/app.go (3 hunks)
  • app/genesis.go (1 hunks)
  • app/keepers/keepers.go (0 hunks)
  • app/upgrade.go (2 hunks)
  • client/docs/config.json (1 hunks)
  • client/docs/swagger-ui/swagger.yaml (2 hunks)
  • go.mod (3 hunks)
  • scripts/protoc-swagger-gen.sh (3 hunks)
💤 Files with no reviewable changes (1)
  • app/keepers/keepers.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Run test and upload codecov
  • GitHub Check: Minitiad
🔇 Additional comments (20)
Dockerfile (1)

9-9: Version update for LIBMOVEVM looks good.

The upgrade from v0.6.0 to v0.7.0 aligns with the overall dependency updates mentioned in the PR objectives.

client/docs/config.json (1)

199-209: Block SDK integration looks good.

The addition of Block SDK's auction and mempool Swagger configurations follows the existing pattern and is properly formatted.

app/upgrade.go (2)

12-12: Version bump approved.

The upgrade from 0.7.0 to 0.7.1 is consistent with the dependency updates in this PR.


33-33: Verify migration handling is properly tested.

The change from simply returning the version map to running migrations is a significant functional change that alters the upgrade flow.

Have you thoroughly tested the migration process to ensure it doesn't cause issues during the upgrade? Particularly, verify that the migration handlers for all modules work as expected with the new version map.

app/genesis.go (1)

126-126: Confirm appropriate gas limit for hooks.

The addition of a 3,000,000 gas limit for hooks seems reasonable, but should be verified against expected hook operations.

Did you benchmark typical hook operations to ensure this gas limit is sufficient but not excessive? This value could impact performance and security in hook execution contexts.

scripts/protoc-swagger-gen.sh (4)

14-15: Block SDK integration properly initialized

The addition of Block SDK URL and version variables follows the established pattern for dependency management in this script.


23-23: Version extraction correctly implemented

The extraction of the Block SDK version from go.mod is consistent with the approach used for other dependencies.


33-33: SDK repository cloning properly configured

The git clone command for Block SDK follows the same pattern as other dependencies, ensuring consistency.


47-47: Proto directory inclusion correctly added

The addition of Block SDK proto directory in the proto_dirs variable ensures proper Swagger generation for the new dependency.

app/app.go (3)

72-73: Block SDK imports properly added

The imports for Block SDK components are correctly added with appropriate versioning (v2).


474-475: Block SDK API routes registration properly implemented

The registration of Block SDK mempool API routes follows the established pattern for service registration within the application.


505-511: Block SDK mempool service integration is complete

The integration includes proper type assertion to ensure the mempool implements the required interface, with appropriate error handling (panic) if the assertion fails.

The mempool service registration follows the same pattern as other service registrations in the application.

client/docs/swagger-ui/swagger.yaml (1)

44990-45142: Detailed Review of Auction & Mempool Endpoints Definition

The Swagger definitions for the /block-sdk/auction/v1/params endpoint and the /block-sdk/mempool/v1/distribution endpoint are very comprehensive. The response schemas are clearly provided with detailed descriptions and proper types for each property.

  • Ensure that the usage of format: byte for fields like escrow_account_address and escrow_address_string is consistent with similar definitions elsewhere in your API.
  • Verify that the use of additionalProperties with type: string and format: uint64 for the distribution field is intentional (rather than a numeric type) so that consumers of the API know what to expect.
  • Double-check that the instructions in the multi-line descriptions (using both >- and |- scalars) are aligned with your preferred YAML style guidelines for consistency.
go.mod (7)

18-19: Dependency Version Bump for Cosmos DB and Cosmos SDK
Both github.com/cosmos/cosmos-db and github.com/cosmos/cosmos-sdk have been bumped to v1.1.1 and v0.50.12 respectively. Please ensure downstream modules and integrations remain fully compatible with these newer versions.


22-22: Packet Forward Middleware Update
The dependency for github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v8 has been updated to v8.1.1. Verify that all API contracts and middleware integration points are still functioning as expected after this change.


28-29: Initia Libraries Upgrade
Both github.com/initia-labs/OPinit and github.com/initia-labs/initia have been updated to v0.7.2. Confirm that these updates do not introduce any breaking changes and that all dependent functionalities are adequately tested post-upgrade.


36-36: MoveVM Dependency Alignment
The version for github.com/initia-labs/movevm has been updated to v0.7.0. This change aligns with the Dockerfile’s LIBMOVEVM_VERSION variable, ensuring consistent versioning across the system.


37-37: Forwarding Module Upgrade
github.com/noble-assets/forwarding/v2 is now at v2.0.1. As this is a patch-level update, please confirm that all interfaces utilizing this module continue to work as expected.


42-42: Cast Library Update
The dependency github.com/spf13/cast has been updated to v1.7.1. Review any associated deprecations or API adjustments to ensure seamless type conversions within the project.


97-97: Ledger Cosmos Go Upgrade (Indirect)
The indirect dependency github.com/cosmos/ledger-cosmos-go has been updated to v0.14.0. Please verify that any ledger integrations relying on this package are unaffected by the update.

Copy link

codecov bot commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 9.09091% with 10 lines in your changes missing coverage. Please review.

Project coverage is 76.89%. Comparing base (ddaf487) to head (9540eb2).

Files with missing lines Patch % Lines
app/app.go 0.00% 9 Missing ⚠️
app/upgrade.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
- Coverage   77.39%   76.89%   -0.51%     
==========================================
  Files          14       14              
  Lines        1420     1428       +8     
==========================================
- Hits         1099     1098       -1     
- Misses        271      280       +9     
  Partials       50       50              
Files with missing lines Coverage Δ
app/genesis.go 90.97% <100.00%> (+0.06%) ⬆️
app/keepers/keepers.go 97.40% <ø> (-0.02%) ⬇️
app/upgrade.go 20.00% <0.00%> (ø)
app/app.go 55.19% <0.00%> (-1.52%) ⬇️

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

🧹 Nitpick comments (1)
client/docs/swagger-ui/swagger.yaml (1)

65169-65181: Minor Typographical Correction in Revision Height Description

There is a minor typo in the description text ("monitonically" should be "monotonically"). This small correction will enhance the clarity and professionalism of the documentation.

Suggested diff:

-      height continues to be monitonically increasing even as the RevisionHeight
+      height continues to be monotonically increasing even as the RevisionHeight
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c1190c and 9540eb2.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (7)
  • .github/workflows/docker.yml (0 hunks)
  • app/app.go (3 hunks)
  • app/genesis.go (1 hunks)
  • app/keepers/keepers.go (0 hunks)
  • app/upgrade.go (2 hunks)
  • client/docs/swagger-ui/swagger.yaml (3 hunks)
  • go.mod (3 hunks)
💤 Files with no reviewable changes (2)
  • .github/workflows/docker.yml
  • app/keepers/keepers.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • app/upgrade.go
  • app/genesis.go
  • go.mod
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Run test and upload codecov
🔇 Additional comments (5)
app/app.go (3)

73-74: Appropriate import additions for Block SDK integration.

The imports for the Block SDK packages are correctly placed under the "skip-mev modules" section, maintaining the code organization.


475-476: Block SDK mempool API routes properly registered.

The integration registers the Block SDK's gRPC gateway routes at the correct point in the application's API registration flow.


506-512: Block SDK mempool service registration looks good.

The code correctly:

  1. Retrieves the application's mempool
  2. Performs a type assertion to ensure it implements the Block.Mempool interface
  3. Registers the mempool service with the application's gRPC query router

The panic is appropriate as a fail-fast mechanism if the mempool does not implement the required interface.

client/docs/swagger-ui/swagger.yaml (2)

44790-44941: New API Endpoints for Auction and Mempool Modules

The newly added endpoints (/block-sdk/auction/v1/params and /block-sdk/mempool/v1/distribution) have well-detailed response schemas, including nested properties for auction parameters and mempool transaction distribution. The descriptions are comprehensive and the use of proper types (e.g. integer with format int64, string with format byte, and boolean) appears consistent.

One suggestion is to verify that the use of type: string with format: uint64 for the mempool distribution values is intentional, as numeric values might typically use a numerical type. If this choice is dictated by backend constraints or client-side requirements, it’s acceptable. Otherwise, consider using a numeric type to better reflect the data.


73000-73135: New Schema Definitions for Auction and Mempool Response Types

The new definitions for sdk.auction.v1.Params, sdk.auction.v1.QueryParamsResponse, and sdk.mempool.v1.GetTxDistributionResponse provide a clear, structured description for the API responses. Their properties and nested object structures mirror the details provided in the endpoint responses, ensuring consistency across the documentation.

A couple of points for review:

  • Double-check that the descriptions, especially multiline texts (using >- and |-), are rendered as intended in Swagger UI.
  • Ensure that these definitions align with the actual backend implementations to avoid discrepancies between the API documentation and runtime behavior.

Overall, these additions enhance the API's clarity and usability.

Copy link
Contributor

@sh-cha sh-cha left a comment

Choose a reason for hiding this comment

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

LGTM

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