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

Problem: require gas for ica packet callback is not adjusted with ctx params #1251

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

mmsqe
Copy link
Collaborator

@mmsqe mmsqe commented Dec 5, 2023

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

  • Have you read the CONTRIBUTING.md?
  • Does your PR follow the C4 patch requirements?
  • Have you rebased your work on top of the latest master?
  • Have you checked your code compiles? (make)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (make test)
  • Have you checked your code formatting is correct? (go fmt)
  • Have you checked your basic code style is fine? (golangci-lint run)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
  • If your changes affect the client infrastructure, have you run the integration test?
  • If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
  • If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
  • If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

Summary by CodeRabbit

  • New Features

    • Introduced a new search bar for enhanced navigation and user experience.
    • Added a new interchain account control transaction method for improved blockchain interactions.
    • Implemented new configuration settings for gas and fee market adjustments.
  • Improvements

    • Enhanced address validation logic in smart contract operations for increased security.
    • Refined interchain communication tests to ensure robustness and reliability.
  • Bug Fixes

    • Adjusted gas requirements for message submissions to optimize transaction processing.
  • Documentation

    • Updated CHANGELOG to reflect recent changes in gas requirements for developers and users.
  • Refactor

    • Modified internal functions to accept additional context parameters, improving the flow and functionality of the application.
  • Tests

    • Added new test cases and fixtures to cover interchain communication features.

Please note that these changes are part of an ongoing effort to improve the application's performance and user experience. Users are encouraged to explore the new features and provide feedback.

Copy link
Contributor

coderabbitai bot commented Dec 5, 2023

Walkthrough

The changes involve updates to a blockchain application, focusing on interchain communication and Ethereum Virtual Machine (EVM) precompiles. Adjustments to gas requirements, context parameters, and configuration settings suggest enhancements to efficiency and functionality. New functionalities for account registration and transaction handling in tests indicate an expansion of testing capabilities. Additionally, there's a shift in the logic for address handling and validation, as well as a restructuring of interface methods, reflecting a refinement in the application's internal operations.

Changes

File Path Change Summary
CHANGELOG.md Adjusted gas requirements for submitMsgs in ICA precompile.
app/app.go Added sdk.Context parameter to evmkeeper.CustomContractFn functions.
integration_tests/configs/ibc.jsonnet Added cronos sub-configuration with max_callback_gas and feemarket+ with no_base_fee.
integration_tests/cosmoscli.py Added ica_ctrl_send_tx method for transaction handling.
integration_tests/ibc_utils.py Added register_acc function for account registration.
integration_tests/test_ica.py Modified register_acc function to accept new arguments.
integration_tests/test_ica_ctrl.py Added imports, a fixture, and a test function for interchain communication.
integration_tests/utils.py Changed a function to return result instead of breaking a loop.
x/cronos/keeper/keeper.go Introduced address conversion and validation logic.
x/cronos/keeper/precompiles/ica.go Added global variable, updated function signatures, and altered control flow for gas calculation.
x/cronos/types/interfaces.go Removed ethtypes import and changed CronosKeeper interface method from CallEVM to GetParams.

Poem

In the code where logic threads, 🐇💻
A rabbit hops, refining the EVM's steads.
Gas and context, in harmony they blend,
For interchain messages to smoothly send. 🌐✉️


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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 a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Merging #1251 (ea43f3c) into main (5971e6e) will decrease coverage by 0.04%.
The diff coverage is 6.66%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1251      +/-   ##
==========================================
- Coverage   35.82%   35.79%   -0.04%     
==========================================
  Files         116      116              
  Lines       10638    10651      +13     
==========================================
+ Hits         3811     3812       +1     
- Misses       6452     6464      +12     
  Partials      375      375              
Files Coverage Δ
x/cronos/keeper/precompiles/ica.go 8.27% <16.66%> (+0.41%) ⬆️
x/cronos/keeper/keeper.go 63.40% <0.00%> (-2.73%) ⬇️

@mmsqe mmsqe marked this pull request as ready for review December 5, 2023 09:10
@mmsqe mmsqe requested a review from a team as a code owner December 5, 2023 09:10
@mmsqe mmsqe requested review from JayT106 and leejw51crypto and removed request for a team December 5, 2023 09:10
x/cronos/keeper/keeper.go Outdated Show resolved Hide resolved
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.

Review Status

Actionable comments generated: 6

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 5971e6e and acaeab2.
Files ignored due to filter (3)
  • go.mod
  • go.sum
  • gomod2nix.toml
Files selected for processing (11)
  • CHANGELOG.md (1 hunks)
  • app/app.go (1 hunks)
  • integration_tests/configs/ibc.jsonnet (1 hunks)
  • integration_tests/cosmoscli.py (1 hunks)
  • integration_tests/ibc_utils.py (1 hunks)
  • integration_tests/test_ica.py (3 hunks)
  • integration_tests/test_ica_ctrl.py (1 hunks)
  • integration_tests/utils.py (1 hunks)
  • x/cronos/keeper/keeper.go (1 hunks)
  • x/cronos/keeper/precompiles/ica.go (4 hunks)
  • x/cronos/types/interfaces.go (2 hunks)
Files skipped from review due to trivial changes (1)
  • integration_tests/configs/ibc.jsonnet
Additional comments: 14
CHANGELOG.md (1)
  • 16-20: The changelog entry for PR #1251 correctly documents the adjustment of required gas for submitMsgs in the ICA precompile as a state machine breaking change.
app/app.go (1)
  • 537-546: The addition of sdk.Context to the function signatures within evmkeeper.CustomContractFn slice is a significant change. Ensure that all calls to these functions have been updated to pass the context as required.
integration_tests/ibc_utils.py (1)
  • 638-639: The function correctly returns the ICA address and channel ID, which aligns with the PR objectives.
integration_tests/test_ica.py (2)
  • 31-37:
    The refactoring of the register_acc function to accept cli_controller and connid as arguments is correctly implemented and used within test_ica.

  • 74-80:
    The logic for checking the balance after transactions, waiting for the channel to be ready, and reopening the ICA account is correctly implemented.

integration_tests/test_ica_ctrl.py (1)
  • 78-78: The assertion for the error message is very specific. Ensure that this message is consistent and won't change with different versions or configurations, as it could make the test brittle.
integration_tests/utils.py (1)
  • 92-99: The wait_for_fn function is well-implemented and follows best practices. It provides a generic way to wait for a condition to be met by a function, with a timeout mechanism to prevent indefinite waiting.
x/cronos/keeper/keeper.go (1)
  • 294-302: The address conversion and validation logic is correctly implemented.
x/cronos/keeper/precompiles/ica.go (4)
  • 34-38: The addition of the icaMethodNamesByID global variable is consistent with the PR objectives and summary.

  • 59-62: The update to the init function to populate icaMethodNamesByID with method names is correct and aligns with the PR objectives.

  • 80-86: The change in the NewIcaContract function signature to include sdk.Context is consistent with the PR objectives and summary.

  • 99-107: The conditional check in the RequiredGas method to adjust the required gas based on the method name is correct and aligns with the PR objectives.

x/cronos/types/interfaces.go (2)
  • 15-20: The summary indicates that the ethtypes import was removed, but it is still present in the provided hunk. This discrepancy should be clarified or corrected.

  • 91-95: The change to the CronosKeeper interface, replacing CallEVM with GetParams, is significant and may affect any code that relies on the old method signature. Ensure that all implementations and usages of CronosKeeper have been updated accordingly.

integration_tests/test_ica_ctrl.py Show resolved Hide resolved
x/cronos/keeper/keeper.go Outdated Show resolved Hide resolved
x/cronos/keeper/keeper.go Show resolved Hide resolved
integration_tests/cosmoscli.py Outdated Show resolved Hide resolved
integration_tests/ibc_utils.py Outdated Show resolved Hide resolved
integration_tests/ibc_utils.py Outdated Show resolved Hide resolved
@mmsqe mmsqe enabled auto-merge December 5, 2023 12:52
@mmsqe mmsqe added this pull request to the merge queue Dec 5, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 5, 2023
@mmsqe mmsqe added this pull request to the merge queue Dec 5, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 5, 2023
@mmsqe mmsqe added this pull request to the merge queue Dec 5, 2023
Merged via the queue into crypto-org-chain:main with commit abc3297 Dec 5, 2023
31 of 32 checks passed
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