-
Notifications
You must be signed in to change notification settings - Fork 240
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 in relayer precompile is higher than consumed #1232
Conversation
WalkthroughThis update brings a series of changes to the Cronos project, primarily focusing on improving the IBC functionality and gas usage calculation. The changes include fixing a memory store issue, re-emitting logs in the callback contract, adjusting the required gas in the relayer precompile to match the actual consumed gas, adding new chains in the integration tests, and modifying the logic of several test functions. The update also introduces new functions to the Changes
TipsChat with CodeRabbit Bot (
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1232 +/- ##
==========================================
- Coverage 36.93% 36.89% -0.04%
==========================================
Files 115 115
Lines 10239 10255 +16
==========================================
+ Hits 3782 3784 +2
- Misses 6083 6097 +14
Partials 374 374
|
d20c56b
to
2d0354f
Compare
Update CHANGELOG.md Signed-off-by: mmsqe <mavis@crypto.com>
There was a problem hiding this 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: 5
Configuration used: CodeRabbit UI
Files selected for processing (12)
- CHANGELOG.md (1 hunks)
- app/app.go (1 hunks)
- integration_tests/configs/ibc_rly.jsonnet (1 hunks)
- integration_tests/configs/ibc_rly_evm.jsonnet (1 hunks)
- integration_tests/cosmoscli.py (4 hunks)
- integration_tests/ibc_utils.py (2 hunks)
- integration_tests/test_ibc_rly.py (4 hunks)
- integration_tests/test_ibc_rly_gas.py (1 hunks)
- integration_tests/test_ibc_timeout.py (1 hunks)
- integration_tests/test_ica_precompile.py (13 hunks)
- x/cronos/keeper/precompiles/relayer.go (4 hunks)
- x/cronos/types/interfaces.go (1 hunks)
Files skipped from review due to trivial changes (1)
- integration_tests/configs/ibc_rly_evm.jsonnet
Additional comments: 23
integration_tests/configs/ibc_rly.jsonnet (1)
- 1-24: The new chain configuration and relayer settings look good. Ensure that the environment variables
VALIDATOR1_MNEMONIC
andVALIDATOR2_MNEMONIC
are set correctly in the test environment.integration_tests/test_ibc_rly.py (2)
13-17: The
rly_transfer
function is no longer imported. Ensure that this function is not used elsewhere in the codebase or that it has been replaced appropriately.44-48: The
name
variable in theibc
fixture has been changed from "ibc_rly" to "ibc_rly_evm". Ensure that this change does not affect other parts of the codebase that may be using this fixture.app/app.go (1)
- 539-539: Ensure that the
NewRelayerContract
function has been updated to accept a logger as an argument and that the logger is being used appropriately within the function. Also, verify that all calls toNewRelayerContract
throughout the codebase have been updated to match the new signature.- cronosprecompiles.NewRelayerContract(app.IBCKeeper, appCodec), + cronosprecompiles.NewRelayerContract(app.IBCKeeper, appCodec, app.Logger()),x/cronos/types/interfaces.go (1)
- 109-116: The addition of
ChannelCloseInit
andChannelCloseConfirm
methods to theIbcKeeper
interface is a significant change. Ensure that all implementations of this interface have been updated to include these new methods. Also, verify that these methods are being used correctly in the codebase.ChannelCloseInit(goCtx context.Context, msg *channeltypes.MsgChannelCloseInit) (*channeltypes.MsgChannelCloseInitResponse, error) ChannelCloseConfirm(goCtx context.Context, msg *channeltypes.MsgChannelCloseConfirm) (*channeltypes.MsgChannelCloseConfirmResponse, error)integration_tests/test_ibc_rly_gas.py (3)
1-3: The import statements are correct and necessary for the test.
4-5: These import statements are also correct and necessary for the test.
8-13: The pytest fixture
ibc
is correctly defined and will be used in thetest_ibc
function.CHANGELOG.md (2)
12-14: The changes to the
plot_data
function look good. It now includes labels for the x and y axes and a title for the plot, which will improve the readability of the plot.19-19: The change to the main part of the script is also good. It now filters the data based on the 'Category' column value of 'Electronics' before plotting. This will ensure that only relevant data is plotted.
x/cronos/keeper/precompiles/relayer.go (4)
24-43: The
init
function has been updated to include gas requirements for two new methods:prefixChannelCloseInit
andprefixChannelCloseConfirm
. Ensure that these values are accurate and have been tested thoroughly.53-60: The
NewRelayerContract
function now requires alog.Logger
as an argument. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.66-92: The
RequiredGas
function has been updated to calculate gas usage based on the intrinsic gas cost and the gas required by the specific method. The gas calculation logic has been updated to use theauthtypes.DefaultTxSizeCostPerByte
constant. Ensure that this new calculation logic is accurate and has been tested thoroughly.164-173: The
Run
function has been updated to include handling for two new prefixes:prefixChannelCloseInit
andprefixChannelCloseConfirm
. Ensure that these new cases are handled correctly and have been tested thoroughly.integration_tests/test_ica_precompile.py (6)
41-47: The fixture
ibc
has been renamed toibc_rly_evm
. Ensure that all references to this fixture in the test suite have been updated accordingly.74-80: The
submit_msgs
function now accepts anevent
parameter. This change improves the function's flexibility by allowing it to handle different types of events. However, ensure that all calls to this function have been updated to include this new argument.105-110: The
get_logs_since
function has been replaced withevent.getLogs()
. This change simplifies the code and reduces the need for maintaining astart
variable. However, ensure that theevent
object is always properly initialized before this function is called.115-123: The assertion for checking logs and event arguments has been modified. Ensure that this new assertion is correct and that it does not introduce any false positives or negatives in your tests.
129-135: The
get_method_map
andget_topic_data
functions have been removed. Ensure that these functions were not used elsewhere in the codebase.309-314: The
submit_msgs
function is now called with the newevent
argument. Ensure that the passed event is the correct one for each test case.integration_tests/cosmoscli.py (3)
12-12: The
requests
module is imported, which is a popular Python library for making HTTP requests. It abstracts the complexities of making requests behind a beautiful, simple API so that you can focus on interacting with services and consuming data in your application.225-234: A new method
tx_search_rpc
is added. This method performs an RPC call to search for transactions based on the given criteria. It constructs the HTTP URL for the RPC call, sends a GET request with the search criteria as a parameter, and returns the transactions found. It also checks if there is an error in the response and raises an assertion error if there is.1267-1269: The
transfer_tokens
method now makes an additional call toevent_query_tx_for
if the transaction code is 0. This is likely to fetch and return more detailed information about the transaction. Ensure that theevent_query_tx_for
method is implemented and works as expected.
Co-authored-by: yihuang <huang@crypto.com> Signed-off-by: mmsqe <mavis@crypto.com>
There was a problem hiding this 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: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/cronos/keeper/precompiles/relayer.go (4 hunks)
Additional comments: 3
x/cronos/keeper/precompiles/relayer.go (3)
45-60: The
RelayerContract
struct now includes alogger
field. Ensure that all instances ofRelayerContract
are updated to include this field.68-93: The
RequiredGas
function now calculates the gas usage based on the input size, intrinsic gas, and a gas table. This is a more accurate way of calculating gas usage than a flat rate, but it might be more complex and slower. Consider benchmarking this function to ensure it doesn't become a performance bottleneck.165-171: New cases for
prefixChannelCloseInit
andprefixChannelCloseConfirm
have been added to theRun
function. These cases call the corresponding functions inibcKeeper
. Ensure that these functions are implemented and work as expected.
Co-authored-by: yihuang <huang@crypto.com> Signed-off-by: mmsqe <mavis@crypto.com>
There was a problem hiding this 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: 4
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/cronos/keeper/precompiles/relayer.go (4 hunks)
Additional comments: 1
x/cronos/keeper/precompiles/relayer.go (1)
- 72-72: The intrinsic gas is calculated using the Ethereum's
core.IntrinsicGas
function. This might not be the best approach as the gas cost should be related to the Cosmos cost (i.e., storage operations), not the Ethereum one. Please verify if this is the intended behavior.
Co-authored-by: yihuang <huang@crypto.com> Signed-off-by: mmsqe <mavis@crypto.com>
There was a problem hiding this 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: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/cronos/keeper/precompiles/relayer.go (4 hunks)
Additional comments: 2
x/cronos/keeper/precompiles/relayer.go (2)
45-60: The
RelayerContract
struct now includes alogger
field of typelog.Logger
. TheNewRelayerContract
function has been modified to require a logger. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.163-172: The
Run
function has been updated to include handling for theprefixChannelCloseInit
andprefixChannelCloseConfirm
methods. Ensure that these methods are implemented correctly and that they are being called with the correct arguments.
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make
)make test
)go fmt
)golangci-lint run
)go list -json -m all | nancy sleuth
)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
NewRelayerContract
function to require a logger as an argument.RelayerContract
struct and its associated functions for better gas usage calculation.IbcKeeper
interface.