Problem: no register support for payee and counterpartyPayee #1665
Conversation
WalkthroughThe changes introduced in this pull request enhance the functionality of the relayer system by adding support for registering payees and counterparty payees. This is reflected in updates to various files, including the addition of new methods in the relayer precompile and modifications to configuration files and tests. The updates maintain the existing structure while introducing new parameters and functionalities to improve payment handling and interchain communication. Changes
Possibly related PRs
Suggested reviewers
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1665 +/- ##
==========================================
+ Coverage 34.99% 35.17% +0.17%
==========================================
Files 123 123
Lines 11783 11740 -43
==========================================
+ Hits 4124 4129 +5
+ Misses 7245 7197 -48
Partials 414 414
|
Signed-off-by: mmsqe <[email protected]>
mmsqe
requested review from
yihuang and
calvinaco
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (9)
integration_tests/test_ibc_rly_gas.py (1)
Line range hint
31-37: Consider strengthening the gas comparison test.The current implementation uses a fixed 10% tolerance for gas comparison. This might be too rigid or too loose depending on the network conditions and operations being tested.
Consider these improvements:
- diff = 0.1 - record = log_gas_records(cli) - if record: - records.append(record) - if len(records) == 2: - res = float(sum(records[0]) / sum(records[1])) - assert 1 - diff <= res <= 1 + diff, res + diff = 0.1 + record = log_gas_records(cli) + if record: + records.append(record) + if len(records) == 2: + res = float(sum(records[0]) / sum(records[1])) + assert 1 - diff <= res <= 1 + diff, \ + f"Gas usage ratio {res:.2f} outside acceptable range [{1-diff:.2f}, {1+diff:.2f}]" + # Log detailed gas usage for analysis + print(f"Gas usage comparison - Test case: {request.param}") + print(f"First run total gas: {sum(records[0])}") + print(f"Second run total gas: {sum(records[1])}") + print(f"Ratio: {res:.2f}")integration_tests/contracts/contracts/TestRelayer.sol (1)
1-31: Add comprehensive NatSpec documentationConsider adding NatSpec documentation to improve the contract's maintainability and usability.
// SPDX-License-Identifier: MIT pragma solidity ^0.8.4; +/// @title TestRelayer +/// @notice Test contract for relayer functionality including payee registration +/// @dev Used for testing relayer precompile interactions contract TestRelayer {x/cronos/events/bindings/src/RelayerFunctions.sol (1)
23-24: Add NatSpec documentation for the new functions.Consider adding comprehensive NatSpec documentation to clarify:
- Function purposes
- Parameter requirements
- Return value meanings
- Expected behavior and error conditions
+ /// @notice Register a payee address for fee distribution + /// @param portID The IBC port identifier + /// @param channelID The IBC channel identifier + /// @param payeeAddr The Ethereum address of the payee + /// @return success True if registration was successful function registerPayee(string calldata portID, string calldata channelID, address payeeAddr) external payable returns (bool); + /// @notice Register a counterparty chain's payee address for fee distribution + /// @param portID The IBC port identifier + /// @param channelID The IBC channel identifier + /// @param counterpartyPayeeAddr The address of the payee on the counterparty chain + /// @return success True if registration was successful function registerCounterpartyPayee(string calldata portID, string calldata channelID, string calldata counterpartyPayeeAddr) external payable returns (bool);integration_tests/configs/ibc.jsonnet (1)
Based on the search results and code analysis, I can now provide a final response:
IBC fee middleware configuration is incomplete and needs additional parameters
The current configuration only includes
ibcfeetoken allocation in accounts but lacks the necessary IBC fee middleware parameters in genesis configuration. To properly support payee registration, add the following to the genesis configuration:
- In
chainmain-1andcronos_777-1sections, add IBC fee middleware parameters underapp_state:app_state+: { fee_middleware+: { params: { enable_fee: true, default_fee_denom: 'ibcfee' } } }🔗 Analysis chain
Line range hint
1-181: Verify the completeness of genesis parameters for IBC fee middleware.The genesis configuration includes new parameters for cronos and feemarket, but might need additional IBC fee middleware configurations to fully support payee registration.
Consider adding:
- IBC fee middleware parameters in the genesis configuration
- Documentation comments explaining the relationship between these parameters and the payee registration feature
Let me gather more information about IBC fee middleware configurations in the genesis parameters.
Let me try a different approach to search for IBC fee middleware configurations.
Let me try one final search pattern to look for IBC middleware configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any IBC fee middleware configurations in other test configs rg -g '*.jsonnet' "ibcfee" integration_tests/configs/Length of output: 234
Script:
#!/bin/bash # Search for fee-related configurations in genesis parameters ast-grep --pattern 'genesis+: { app_state+: { $$$ } }' # Also check for any other fee middleware related configurations rg -g '*.jsonnet' -i "fee.*middleware|middleware.*fee" integration_tests/configs/Length of output: 148
Script:
#!/bin/bash # Search for any fee-related configurations in app_state rg -g '*.jsonnet' "app_state.*fee" integration_tests/configs/ -A 5 # Search for any IBC-related configurations in app_state rg -g '*.jsonnet' "app_state.*ibc" integration_tests/configs/ -A 5Length of output: 137
Script:
#!/bin/bash # Search for middleware configurations in the entire codebase rg -g '*.jsonnet' "middleware" integration_tests/configs/ -B 2 -A 5 # Look for any files that might contain IBC configurations fd -e jsonnet . integration_tests/configs/ --exec head -n 50 {}Length of output: 18423
x/cronos/types/interfaces.go (1)
99-102: Consider adding query methods to the interfaceThe interface currently only supports registration operations. Consider adding query methods to retrieve the registered payee and counterparty payee information, which would be useful for verification and display purposes.
Example methods to consider:
GetPayeeAddress(goCtx context.Context, channelID string) (string, error) GetCounterpartyPayeeAddress(goCtx context.Context, channelID string) (string, error)integration_tests/test_ica_precompile.py (1)
Line range hint
186-187: Consider enhancing test documentation.The test functions would benefit from descriptive docstrings explaining:
- The purpose of each test case
- The scenarios being tested (ordered vs unordered)
- Expected outcomes
Example improvement:
@pytest.mark.parametrize("order", [Ordering.ORDERED.value, Ordering.UNORDERED.value]) +def test_call(ibc, order): + """Test ICA message submission with different channel ordering. + + Args: + ibc: IBC test fixture + order: Channel ordering (ORDERED or UNORDERED) + + Tests: + - Account registration + - Message submission + - Balance verification + """Also applies to: 307-308
CHANGELOG.md (1)
3-8: LGTM! Consider adding more details about the feature impact.The changelog entry is well-formatted and properly references the PR. However, consider expanding the description to briefly explain the benefits or impact of this feature for users/developers.
Example addition:
-* [#1665](https://github.com/crypto-org-chain/cronos/pull/1665) Support register for payee and counterpartyPayee in relayer precompile. +* [#1665](https://github.com/crypto-org-chain/cronos/pull/1665) Support register for payee and counterpartyPayee in relayer precompile, enabling automated fee collection for relayer operations.integration_tests/cosmoscli.py (1)
891-891: LGTM! Consider enhancing the parameter documentation.The change to make
target_versionoptional with a default value of 1 is a good improvement for usability while maintaining backward compatibility.Consider updating the parameter documentation to clarify when the default value is appropriate and when it should be explicitly set:
- target_version=1, # chain version number of target chain + target_version=1, # chain version number of target chain (default: 1 for basic IBC transfers)integration_tests/ibc_utils.py (1)
195-195: Use a descriptive constant for the fund amountAt line 195, the fund amount is hard-coded:
fund = 3000000000000000000Defining this value as a constant with a descriptive name improves code readability and maintainability.
Apply this diff to introduce a constant:
+FUND_AMOUNT_WEI = 3 * 10**18 # 3 Ether in Wei -fund = 3000000000000000000 +fund = FUND_AMOUNT_WEI
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- CHANGELOG.md (1 hunks)
- app/app.go (1 hunks)
- integration_tests/configs/ibc.jsonnet (2 hunks)
- integration_tests/contracts/contracts/TestRelayer.sol (1 hunks)
- integration_tests/cosmoscli.py (1 hunks)
- integration_tests/ibc_utils.py (10 hunks)
- integration_tests/test_ibc.py (1 hunks)
- integration_tests/test_ibc_rly.py (1 hunks)
- integration_tests/test_ibc_rly_gas.py (1 hunks)
- integration_tests/test_ica_precompile.py (1 hunks)
- x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go (2 hunks)
- x/cronos/events/bindings/src/RelayerFunctions.sol (1 hunks)
- x/cronos/keeper/precompiles/relayer.go (6 hunks)
- x/cronos/types/interfaces.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go
[warning] 459-460: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go#L459-L460
Added lines #L459 - L460 were not covered by tests
[warning] 466-467: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go#L466-L467
Added lines #L466 - L467 were not covered by tests
[warning] 473-474: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go#L473-L474
Added lines #L473 - L474 were not covered by tests
[warning] 480-481: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go#L480-L481
Added lines #L480 - L481 were not covered by tests
[warning] 487-488: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go#L487-L488
Added lines #L487 - L488 were not covered by tests
[warning] 494-495: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go#L494-L495
Added lines #L494 - L495 were not covered by testsx/cronos/keeper/precompiles/relayer.go
[warning] 203-222: x/cronos/keeper/precompiles/relayer.go#L203-L222
Added lines #L203 - L222 were not covered by tests
[warning] 224-227: x/cronos/keeper/precompiles/relayer.go#L224-L227
Added lines #L224 - L227 were not covered by tests
[warning] 229-229: x/cronos/keeper/precompiles/relayer.go#L229
Added line #L229 was not covered by tests
[warning] 268-268: x/cronos/keeper/precompiles/relayer.go#L268
Added line #L268 was not covered by tests
[warning] 270-270: x/cronos/keeper/precompiles/relayer.go#L270
Added line #L270 was not covered by tests
🔇 Additional comments (16)
integration_tests/test_ibc_rly_gas.py (1)
20-20: Verify integration with payee registration changes.The fixture now conditionally enables the relayer caller for EVM-based tests. Let's verify this change is properly integrated with the new payee registration functionality.
✅ Verification successful
The conditional relayer caller setup is correctly integrated
The verification shows that the
need_relayer_callerparameter inprepare_networkis properly integrated with the payee registration functionality:
- In
test_ibc_rly_gas.py, it's correctly used to enable the relayer caller only for "ibc_rly_evm" tests- The same parameter is used consistently in other IBC-related tests:
test_ica_precompile.pyuses it for ICA (Inter-Chain Account) teststest_ibc_rly.pyenables it for EVM relayer testsThe change is part of a well-structured test suite that verifies different IBC functionalities with appropriate relayer configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for related changes in network preparation and payee registration # Test 1: Check if prepare_network function handles the new parameter rg -A 5 "def prepare_network" # Test 2: Look for payee registration logic in EVM relayer tests rg -A 10 "ibc_rly_evm.*prepare" # Test 3: Check for any payee registration tests rg "test.*payee.*registration"Length of output: 398
Script:
#!/bin/bash # Let's check the actual implementation of prepare_network and its usage # Check the full implementation of prepare_network rg -B 5 -A 20 "def prepare_network" integration_tests/ibc_utils.py # Check the test file content to understand the context better cat integration_tests/test_ibc_rly_gas.py # Look for any EVM relayer specific configurations rg -l "ibc_rly_evm"Length of output: 1966
Script:
#!/bin/bash # Let's check the other files that use ibc_rly_evm to understand the full context # Check the ICA precompile test cat integration_tests/test_ica_precompile.py # Check the IBC relay test cat integration_tests/test_ibc_rly.py # Check the configuration in conftest cat integration_tests/conftest.pyLength of output: 29160
integration_tests/contracts/contracts/TestRelayer.sol (1)
4-10: LGTM! Well-structured contract setupThe changes improve code clarity by:
- Using proper interface typing for relayer interactions
- Clear naming distinction between contract address and interface instance
- Appropriate state variables for tracking registration status
x/cronos/events/bindings/src/RelayerFunctions.sol (3)
23-24: LGTM! The new payee registration functions align with the PR objectives.The interface additions look good and provide the necessary functionality for payee registration in both local and counterparty contexts.
23-24: Specify validation requirements for input parameters.The interface should specify requirements for:
- Valid format/length for
portIDandchannelID- Address format requirements for
counterpartyPayeeAddr- Whether empty/zero values are allowed
23-24: 🛠️ Refactor suggestionConsider standardizing return types across the interface.
Most functions in this interface return
bytes, but the new functions returnbool. Consider:
- Either updating these to return
bytesfor consistency- Or documenting why these functions specifically return
boolintegration_tests/configs/ibc.jsonnet (2)
42-42: Verify the security implications of zero minimum gas price.Setting
minimum-gas-pricesto '0basecro' removes the spam protection mechanism. While this might be intentional for testing, ensure this is not propagated to production configurations.Consider:
- Adding a comment explaining why minimum gas price is set to zero
- Using a small non-zero value that doesn't interfere with tests but maintains basic spam protection
71-71: LGTM: IBC fee token allocation aligns with payee registration feature.The addition of '100000000000ibcfee' tokens to signer2's account is consistent with the PR's objective of adding payee registration support. This will enable testing of the IBC fee middleware functionality.
x/cronos/types/interfaces.go (2)
18-18: LGTM: Import is properly organizedThe new import for
ibcfeetypesis correctly placed with other IBC-related imports and uses the appropriate versioning.
99-102: LGTM: Well-structured interface definitionThe
IbcFeeKeeperinterface is well-defined and follows consistent patterns with other keeper interfaces in the codebase.integration_tests/test_ibc.py (1)
98-98: LGTM! Verify integration with relayer system changes.The update to use
ibc.cronosandibc.chainmainin theregister_fee_payeecall aligns with the PR's objective of adding register support for payees.Let's verify the integration with the relayer system changes:
✅ Verification successful
Let me gather more context about the implementation to ensure the parameters are being used correctly.
Integration verified - parameters match implementation
The
register_fee_payeecall intest_ibc.pycorrectly usesibc.cronosandibc.chainmainas source and destination chains, which matches the function's implementation inibc_utils.py. The function properly handles these parameters to register counterparty payees using the chains' respective CLI interfaces.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the register_fee_payee function implementation and its usage # across the codebase to ensure consistent parameter passing. # Test 1: Find the register_fee_payee function definition echo "Searching for register_fee_payee function definition..." rg -p "def register_fee_payee" # Test 2: Find all calls to register_fee_payee echo "Searching for register_fee_payee function calls..." rg -p "register_fee_payee\("Length of output: 664
Script:
#!/bin/bash # Check the implementation of register_fee_payee and its context echo "Checking register_fee_payee implementation..." rg -p -B 5 -A 10 "def register_fee_payee" integration_tests/ibc_utils.py # Check the test context where it's being called echo "Checking test context..." rg -p -B 5 -A 5 "register_fee_payee" integration_tests/test_ibc.pyLength of output: 1536
integration_tests/test_ibc_rly.py (2)
53-53: LGTM! The change aligns with the PR objectives.The modification from explicit relayer configuration to a boolean flag provides better flexibility for supporting payee registration functionality while maintaining the existing test structure.
53-53: Verify prepare_network compatibility with need_relayer_caller parameter.The change from explicit relayer configuration to a boolean flag needs verification to ensure the
prepare_networkfunction properly handles this parameter.Let's verify the parameter handling in prepare_network:
✅ Verification successful
Let me gather more context about how the
need_relayer_callerparameter is used in the function.
Based on the gathered information, I can now provide the final response:
The need_relayer_caller parameter is properly handled and compatible
The
prepare_networkfunction inibc_utils.pycorrectly implements theneed_relayer_callerparameter:
- It's defined as an optional parameter with a default value of
False- When
True, it properly sets up a new account and deploys a TestRelayer contract- The change aligns with existing usage in other test files (test_ibc_rly_gas.py, test_ica_precompile.py)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if prepare_network function properly handles need_relayer_caller parameter # Expected: Find the prepare_network function definition and verify parameter usage # Search for prepare_network function definition echo "Searching for prepare_network function definition..." rg -A 10 "def prepare_network" integration_tests/ # Search for other usages of need_relayer_caller echo "Searching for other usages of need_relayer_caller..." rg "need_relayer_caller" integration_tests/Length of output: 1391
Script:
#!/bin/bash # Get more context around the usage of need_relayer_caller in ibc_utils.py rg -B 5 -A 15 "if need_relayer_caller:" integration_tests/ibc_utils.pyLength of output: 1024
integration_tests/test_ica_precompile.py (2)
59-59: LGTM: Network preparation configuration is appropriate.The addition of
need_relayer_caller=Truealigns with the PR's objective of supporting payee registration in the relayer system.
Line range hint
65-75: LGTM: Function parameter additions enhance flexibility.The new parameters
contract_addrandwith_channel_idprovide better control over account registration and message submission while maintaining backward compatibility.Also applies to: 89-106
app/app.go (1)
683-683: LGTM! Verify integration with IBC fee module.The addition of
IBCFeeKeepertoNewRelayerContractis correct and aligns with the PR objectives of adding payee registration support. The keeper is properly initialized and follows the dependency injection pattern.Let's verify the integration with the IBC fee module:
✅ Verification successful
Integration with IBC fee module is properly implemented
The verification confirms that the addition of
IBCFeeKeepertoNewRelayerContractis correctly integrated:
- The RelayerContract struct properly includes the
ibcFeeKeeperfield- The contract implements fee-related functionality:
RegisterPayeemethod for registering local payee addressesRegisterCounterpartyPayeemethod for registering counterparty payee addressesDistributeFeeevent for fee distribution trackingThe implementation aligns with the PR objectives of adding payee registration support through the IBC fee module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the relayer contract's integration with IBC fee module # Test: Check for any fee-related methods in the relayer contract ast-grep --pattern $'contract Relayer { $$$ function registerPayee($$$) { $$$ } $$$ }' # Test: Check for any fee keeper usage in the relayer contract implementation rg -A 5 "IBCFeeKeeper.*registerPayee"Length of output: 139
Script:
#!/bin/bash # Let's try a different approach to verify the integration # Check for relayer contract files fd -e sol "Relayer" # Look for any IBC fee-related imports and usage rg -A 5 "IBCFeeKeeper" # Check for any Go files containing relayer contract implementation fd -e go "relayer" # Look for the actual NewRelayerContract implementation rg -A 10 "func NewRelayerContract"Length of output: 5087
Script:
#!/bin/bash # Let's check the RelayerContract implementation for fee-related methods rg -A 10 "type RelayerContract struct" x/cronos/keeper/precompiles/relayer.go # Check for any fee-related methods in the relayer contract rg -l "func.*Payee|func.*Fee" x/cronos/keeper/precompiles/relayer.go # Look at the actual implementation of fee-related methods rg -A 20 "func \(c \*RelayerContract\).*Payee|func \(c \*RelayerContract\).*Fee" x/cronos/keeper/precompiles/relayer.goLength of output: 529
Script:
#!/bin/bash # Let's check the interface definition for fee-related methods rg -A 10 "interface IRelayer" x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go # Check the generated bindings for fee-related methods rg -A 10 "RegisterPayee|RegisterCounterpartyPayee" x/cronos/events/bindings/cosmos/precompile/relayer/ # Look for any fee-related events or types rg -A 5 "Payee|Fee" x/cronos/events/bindings/src/Relayer.solLength of output: 8010
integration_tests/ibc_utils.py (1)
469-473: Confirm that zero fees are acceptable in the network configurationIn the
ibc_transferfunction call, zero fees are specified:fees=f"0{base_denom0}",Ensure that the network allows transactions with zero fees. If zero fees are not permitted, the transaction may fail or be rejected. If necessary, specify a minimal non-zero fee amount to comply with network requirements.
x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go
Show resolved
Hide resolved
x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
integration_tests/ibc_utils.py (2)
188-201: Consider extracting the hardcoded fund amount as a constantThe contract deployment logic is well-structured, but the fund amount
3000000000000000000should be extracted as a named constant for better maintainability.+RELAYER_FUND_AMOUNT = 3000000000000000000 if need_relayer_caller: acc = derive_new_account(2) sender = acc.address # fund new sender to deploy contract with same address if w3.eth.get_balance(sender, "latest") == 0: - fund = 3000000000000000000 + fund = RELAYER_FUND_AMOUNT tx = {"to": sender, "value": fund, "gasPrice": w3.eth.gas_price}
228-273: Add type hints and docstring to improve code maintainabilityThe
register_fee_payeefunction would benefit from type hints and documentation.-def register_fee_payee(src_chain, dst_chain, contract=None, acc=None): +def register_fee_payee( + src_chain: Cronos, + dst_chain: Chainmain, + contract: Optional[Contract] = None, + acc: Optional[Account] = None +) -> None: + """Register fee payee for IBC transfers. + + Args: + src_chain: Source chain instance + dst_chain: Destination chain instance + contract: Optional contract instance for contract-based registration + acc: Optional account for contract transactions + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- integration_tests/ibc_utils.py (10 hunks)
- integration_tests/test_ibc_rly.py (3 hunks)
- integration_tests/test_ibc_rly_gas.py (2 hunks)
- x/cronos/keeper/precompiles/relayer.go (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- integration_tests/test_ibc_rly.py
- integration_tests/test_ibc_rly_gas.py
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/cronos/keeper/precompiles/relayer.go
[warning] 203-222: x/cronos/keeper/precompiles/relayer.go#L203-L222
Added lines #L203 - L222 were not covered by tests
[warning] 224-227: x/cronos/keeper/precompiles/relayer.go#L224-L227
Added lines #L224 - L227 were not covered by tests
[warning] 229-229: x/cronos/keeper/precompiles/relayer.go#L229
Added line #L229 was not covered by tests
[warning] 268-268: x/cronos/keeper/precompiles/relayer.go#L268
Added line #L268 was not covered by tests
[warning] 270-270: x/cronos/keeper/precompiles/relayer.go#L270
Added line #L270 was not covered by tests
🔇 Additional comments (5)
x/cronos/keeper/precompiles/relayer.go (3)
110-116: LGTM! Clean struct and constructor implementation.The addition of
ibcFeeKeeperand its initialization in the constructor are well-implemented and maintain consistency with the existing pattern.Also applies to: 119-129
268-268: LGTM! Clean simplification of method handling.The direct method calls to
ibcKeeperimprove code clarity and maintainability.Also applies to: 270-270
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 268-268: x/cronos/keeper/precompiles/relayer.go#L268
Added line #L268 was not covered by tests
203-228: 🛠️ Refactor suggestionConsider simplifying the payee registration logic.
The current implementation could be simplified by extracting the common logic for both registration methods. This would improve maintainability and reduce duplication.
Consider refactoring to:
if method.Name == RegisterPayee || method.Name == RegisterCounterpartyPayee { execErr := stateDB.ExecuteNativeAction(precompileAddr, converter, func(ctx sdk.Context) error { portID := args[0].(string) channelID := args[1].(string) caller := sdk.AccAddress(contract.CallerAddress.Bytes()).String() - if method.Name == RegisterPayee { - payeeAddr := sdk.AccAddress(args[2].(common.Address).Bytes()).String() - _, err := bc.ibcFeeKeeper.RegisterPayee( - ctx, - ibcfeetypes.NewMsgRegisterPayee(portID, channelID, caller, payeeAddr), - ) - return err - } else { - counterpartyPayeeAddr := args[2].(string) - _, err := bc.ibcFeeKeeper.RegisterCounterpartyPayee( - ctx, - ibcfeetypes.NewMsgRegisterCounterpartyPayee(portID, channelID, caller, counterpartyPayeeAddr), - ) - return err - } + var msg sdk.Msg + if method.Name == RegisterPayee { + payeeAddr := sdk.AccAddress(args[2].(common.Address).Bytes()).String() + msg = ibcfeetypes.NewMsgRegisterPayee(portID, channelID, caller, payeeAddr) + } else { + counterpartyPayeeAddr := args[2].(string) + msg = ibcfeetypes.NewMsgRegisterCounterpartyPayee(portID, channelID, caller, counterpartyPayeeAddr) + } + _, err := bc.ibcFeeKeeper.RegisterPayee(ctx, msg) + return err }) if execErr != nil { return nil, execErr } return method.Outputs.Pack(true) }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 203-222: x/cronos/keeper/precompiles/relayer.go#L203-L222
Added lines #L203 - L222 were not covered by tests
[warning] 224-227: x/cronos/keeper/precompiles/relayer.go#L224-L227
Added lines #L224 - L227 were not covered by testsintegration_tests/ibc_utils.py (2)
149-150: LGTM: Parameter addition for relayer caller supportThe addition of
need_relayer_callerparameter with proper logging enhances the test setup flexibility.Also applies to: 157-157
581-581: LGTM: Enhanced return values for better test assertionsThe function now returns additional fee parameters (
recv_fee,ack_fee) which allows for more precise assertions in test cases.
27f1a59
👮🏻👮🏻👮🏻 !!!! 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
Release Notes
New Features
Bug Fixes
Documentation
These updates enhance the payment handling capabilities and streamline the testing environment for users.