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: no packet info for indexed field in ibc relayer event #1662

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

mmsqe
Copy link
Collaborator

@mmsqe mmsqe commented Oct 23, 2024

👮🏻👮🏻👮🏻 !!!! 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

Release Notes

  • New Features

    • Enhanced event handling with additional packet information for IBC relayer events.
    • Support for parallel generation of test transactions and improved load generation with retry capabilities.
  • Improvements

    • Updated event signatures to utilize uint256 for packetSequence, improving data handling.
    • Added new attributes for source port and channel information in relevant events.
  • Bug Fixes

    • Resolved issues with validator benchmarks and node shutdown processes.
    • Fixed state overwrites in debug trace APIs and removed conflicts in benchmark transactions.
  • Documentation

    • Updated CHANGELOG.md to reflect recent changes and improvements.

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 37 lines in your changes missing coverage. Please review.

Project coverage is 35.01%. Comparing base (1ae61b4) to head (52cc002).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...smos/precompile/relayer/i_relayer_module.abigen.go 0.00% 28 Missing ⚠️
x/cronos/events/events.go 0.00% 6 Missing ⚠️
x/cronos/events/event.go 0.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1662      +/-   ##
==========================================
+ Coverage   34.91%   35.01%   +0.09%     
==========================================
  Files         123      123              
  Lines       11811    11778      -33     
==========================================
  Hits         4124     4124              
+ Misses       7273     7240      -33     
  Partials      414      414              
Files with missing lines Coverage Δ
.../events/bindings/cosmos/lib/cosmos_types.abigen.go 0.00% <ø> (ø)
x/cronos/events/event.go 34.17% <0.00%> (-1.35%) ⬇️
x/cronos/events/events.go 23.07% <0.00%> (-4.20%) ⬇️
...smos/precompile/relayer/i_relayer_module.abigen.go 0.00% <0.00%> (ø)

Comment on lines +71 to +73
for k, v := range replaceAttrs {
attrs[k] = attrs[v]
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
@yihuang yihuang marked this pull request as ready for review October 23, 2024 15:49
@yihuang yihuang requested a review from a team as a code owner October 23, 2024 15:49
@yihuang yihuang requested review from JayT106 and leejw51crypto and removed request for a team October 23, 2024 15:49
Copy link
Contributor

coderabbitai bot commented Oct 23, 2024

Walkthrough

The pull request includes updates to the CHANGELOG.md and several code files, focusing on bug fixes and enhancements across the project. Key changes involve improvements in benchmark functionalities, modifications to event handling in the IBC protocol, and updates to data types in smart contract bindings. Notable additions include new packet information for IBC relayer events and enhancements to the event conversion logic. The changes also reflect updates to the ABI definitions and event signatures in smart contracts, ensuring better data handling and clarity.

Changes

File Change Summary
CHANGELOG.md Updated to include bug fixes and improvements, such as resolving single validator benchmark issues, enhancing benchmark functionality, and adding detailed packet information for IBC relayer events.
integration_tests/test_ibc_rly.py Removed keccak hashing for several fields, replaced with direct assignments. Added new fields packetSrcPortInfo and packetSrcChannelInfo. Updated filter_logs_since to format packet_sequence as a 64-character hex string.
x/cronos/events/bindings/cosmos/lib/cosmos_types.abigen.go Updated Bin field in CosmosTypesMetaData with a new bytecode string.
x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go Updated ABI definitions for events, changed data types for indexed parameters from string to uint256, added new fields for source port and channel info, and modified function signatures accordingly.
x/cronos/events/bindings/src/Relayer.sol Modified IRelayerModule interface to change event parameter types from string to uint256 for packetSequence and updated several other event parameters.
x/cronos/events/event.go Added a new parameter replaceAttrs to ConvertEvent method in EventDescriptor struct for dynamic attribute replacement.
x/cronos/events/events.go Updated RelayerValueDecoders to include new mappings for srcPortInfo and srcChannelInfo, modified attribute handling in RelayerConvertEvent and IcaConvertEvent functions to include replaceAttrs.
x/cronos/events/types/types.go Added constants AttributeKeySrcPortInfo and AttributeKeySrcChannelInfo.

Possibly related PRs

Suggested reviewers

  • devashishdxt
  • thomas-nguy

Poem

🐇 In the meadow, changes bloom,
Bugs fixed, like flowers in room.
With packets bright, and channels clear,
Our code hops forward, full of cheer!
So let us dance, with joy and glee,
For every change, a victory! 🌼


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.

@yihuang yihuang added this pull request to the merge queue Oct 23, 2024
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.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (7)
x/cronos/events/types/types.go (1)

4-7: LGTM! Consider adding documentation.

The new constants follow consistent naming patterns and effectively address the missing packet information issue for IBC relayer events. The implementation aligns well with the existing codebase structure.

Consider adding a comment block above the const block to document the purpose of these event attributes, particularly explaining their role in IBC packet handling. This would help future maintainers understand the context better.

 package types
 
+// Event types and attribute keys for IBC packet handling events.
+// These constants are used to identify and track packet information
+// in the context of Inter-Blockchain Communication (IBC) relayer events.
 const (
 	EventTypeSubmitMsgsResult  = "submit_msgs_result"
 	AttributeKeySeq            = "seq"
 	AttributeKeySrcPortInfo    = "packet_src_port_info"
 	AttributeKeySrcChannelInfo = "packet_src_channel_info"
 )
x/cronos/events/event.go (1)

71-73: Add test coverage for attribute replacement logic.

While the map iteration is safe here (order-independence), the new attribute replacement logic needs test coverage to ensure reliability.

Would you like me to help generate test cases for the attribute replacement functionality? Consider cases like:

  1. Basic attribute replacement
  2. Non-existent source attributes
  3. Multiple replacements
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 71-73: x/cronos/events/event.go#L71-L73
Added lines #L71 - L73 were not covered by tests

🪛 GitHub Check: CodeQL

[warning] 71-73: Iteration over map
Iteration over map may be a possible source of non-determinism

x/cronos/events/events.go (1)

64-76: Consider documenting the attribute replacement pattern.

The new attribute replacement mechanism is a flexible solution that could be valuable for other event types. Consider:

  1. Adding documentation explaining when and how to use this pattern
  2. Creating a shared constant file for common replacement mappings if this pattern expands to other event types
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 64-68: x/cronos/events/events.go#L64-L68
Added lines #L64 - L68 were not covered by tests

CHANGELOG.md (1)

21-21: Consider adding more details to the changelog entry.

While the entry correctly documents the improvement, it could be more specific about what packet information was added for better clarity.

Consider expanding the entry to:

-* [#1662](https://github.com/crypto-org-chain/cronos/pull/1662) Emit more packet info for ibc relayer event.
+* [#1662](https://github.com/crypto-org-chain/cronos/pull/1662) Emit more packet info (source port and channel) for ibc relayer event.
x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go (3)

Line range hint 1581-1621: Consider adding tests for the updated IbcTransfer event handling

The RelayerModuleIbcTransfer struct and associated functions have been modified but lack test coverage. Implementing tests will ensure functionality works as intended.

Would you like assistance in creating tests for this updated code?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 1589-1589: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1589
Added line #L1589 was not covered by tests


Line range hint 1748-1774: Consider adding tests for FilterRecvPacket and WatchRecvPacket functions

These functions have been updated to accommodate new parameters but are not covered by tests. Adding tests will validate the new logic.

Would you like assistance in generating tests for these functions?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 1774-1774: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1774
Added line #L1774 was not covered by tests


Line range hint 2048-2087: Consider adding tests for TimeoutPacket event and associated functions

The RelayerModuleTimeoutPacket struct and related functions have been updated. Tests should be added to ensure the reliability of these changes.

Would you like assistance in creating tests for this code?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 2062-2062: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L2062
Added line #L2062 was not covered by tests

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1ae61b4 and 52cc002.

📒 Files selected for processing (8)
  • CHANGELOG.md (1 hunks)
  • integration_tests/test_ibc_rly.py (6 hunks)
  • x/cronos/events/bindings/cosmos/lib/cosmos_types.abigen.go (1 hunks)
  • x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go (32 hunks)
  • x/cronos/events/bindings/src/Relayer.sol (1 hunks)
  • x/cronos/events/event.go (1 hunks)
  • x/cronos/events/events.go (2 hunks)
  • x/cronos/events/types/types.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go

[warning] 279-279: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L279
Added line #L279 was not covered by tests


[warning] 304-304: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L304
Added line #L304 was not covered by tests


[warning] 1152-1152: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1152
Added line #L1152 was not covered by tests


[warning] 1154-1154: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1154
Added line #L1154 was not covered by tests


[warning] 1164-1164: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1164
Added line #L1164 was not covered by tests


[warning] 1166-1166: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1166
Added line #L1166 was not covered by tests


[warning] 1287-1287: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1287
Added line #L1287 was not covered by tests


[warning] 1294-1294: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1294
Added line #L1294 was not covered by tests


[warning] 1304-1304: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1304
Added line #L1304 was not covered by tests


[warning] 1311-1311: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1311
Added line #L1311 was not covered by tests


[warning] 1434-1434: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1434
Added line #L1434 was not covered by tests


[warning] 1445-1445: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1445
Added line #L1445 was not covered by tests


[warning] 1455-1455: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1455
Added line #L1455 was not covered by tests


[warning] 1466-1466: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1466
Added line #L1466 was not covered by tests


[warning] 1589-1589: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1589
Added line #L1589 was not covered by tests


[warning] 1600-1600: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1600
Added line #L1600 was not covered by tests


[warning] 1610-1610: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1610
Added line #L1610 was not covered by tests


[warning] 1621-1621: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1621
Added line #L1621 was not covered by tests


[warning] 1749-1749: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1749
Added line #L1749 was not covered by tests


[warning] 1774-1774: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1774
Added line #L1774 was not covered by tests


[warning] 1911-1911: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1911
Added line #L1911 was not covered by tests


[warning] 1918-1918: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1918
Added line #L1918 was not covered by tests


[warning] 1928-1928: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1928
Added line #L1928 was not covered by tests


[warning] 1935-1935: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1935
Added line #L1935 was not covered by tests


[warning] 2062-2062: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L2062
Added line #L2062 was not covered by tests


[warning] 2087-2087: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L2087
Added line #L2087 was not covered by tests


[warning] 2384-2384: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L2384
Added line #L2384 was not covered by tests


[warning] 2409-2409: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L2409
Added line #L2409 was not covered by tests

x/cronos/events/event.go

[warning] 71-73: x/cronos/events/event.go#L71-L73
Added lines #L71 - L73 were not covered by tests

x/cronos/events/events.go

[warning] 64-68: x/cronos/events/events.go#L64-L68
Added lines #L64 - L68 were not covered by tests

🪛 GitHub Check: CodeQL
x/cronos/events/event.go

[warning] 71-73: Iteration over map
Iteration over map may be a possible source of non-determinism

🔇 Additional comments (27)
x/cronos/events/bindings/src/Relayer.sol (5)

13-13: Improved type safety for packet sequence numbers.

The change from string to uint256 for packetSequence is a good improvement as it:

  • Provides better type safety for sequence numbers
  • Reduces gas costs compared to string storage
  • Enables numeric comparisons and arithmetic operations

Also applies to: 24-24, 35-35, 45-45


57-57: Appropriate removal of string indexing.

Removing the indexed keyword from string parameters (refundDenom, denom) is beneficial because:

  • String indexing requires keccak256 hashing, which increases gas costs
  • The full string value is still available in the event data
  • Consistent with best practices for handling denomination strings

Also applies to: 63-63, 69-69


73-73: Consistent event parameter indexing.

The removal of indexed from string parameters in DenominationTrace and DistributeFee events maintains consistency with other similar changes in the interface.

Also applies to: 77-77


13-77: Verify event changes in dependent systems.

The changes improve the IBC relayer events by adding detailed packet information and optimizing parameter types. However, these changes might affect systems that consume these events.

#!/bin/bash
# Look for event consumers that might need updates
rg -l "RecvPacket|WriteAcknowledgement|AcknowledgePacket|TimeoutPacket" | grep -v "Relayer.sol"

Ensure that:

  1. Event consumers are updated to handle the new uint256 packet sequence
  2. Systems processing these events can handle the additional info fields
  3. Event indexing services are updated to reflect the removed string indexing

16-17: Consider adding maximum length for info fields.

The new packetSrcPortInfo and packetSrcChannelInfo string fields could benefit from length constraints to prevent excessive gas costs.

Consider:

  1. Adding validation in the implementation to enforce reasonable maximum lengths
  2. Documenting the expected format and size limits in comments

Also applies to: 27-28, 38-39, 48-49

x/cronos/events/event.go (1)

65-65: LGTM: Parameter addition aligns with PR objective.

The new replaceAttrs parameter enables flexible attribute mapping, which addresses the missing packet info issue.

Let's verify the parameter usage across the codebase:

✅ Verification successful

Let me try a different approach to verify the usage of ConvertEvent across the codebase.

This will help us:

  1. Find all occurrences of ConvertEvent with surrounding context
  2. Locate all event-related files that might contain the implementation or usage

Based on the search results, I can now provide a final response:

Parameter addition requires no further updates in the codebase

The verification shows that the ConvertEvent method is used in two places:

  • RelayerConvertEvent which correctly passes the replaceAttrs parameter with the required channel mappings
  • IcaConvertEvent which passes an empty map map[string]string{} as expected

The implementation is consistent across all usages and no additional updates are needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for ConvertEvent calls to ensure they're updated with the new parameter
ast-grep --pattern 'ConvertEvent($$$)'

Length of output: 40


Script:

#!/bin/bash
# Search for ConvertEvent using ripgrep for a broader search
rg "ConvertEvent" -A 2 -B 2

# Also search for files containing "event" in their name to ensure we don't miss any implementations
fd "event" --type f

Length of output: 2122

x/cronos/events/events.go (2)

76-76: LGTM: Empty map parameter is appropriate for ICA events.

The addition of an empty map parameter maintains API consistency while correctly indicating that ICA events don't require attribute replacement.


28-32: Sequence type conversion change looks good, but needs verification.

The change from ReturnStringAsIs to ConvertUint64 for AttributeKeySequence is a good improvement for type safety. The new port info mappings are also correctly implemented.

Let's verify the sequence type usage in the codebase:

x/cronos/events/bindings/cosmos/lib/cosmos_types.abigen.go (2)

41-41: Verify compatibility with Solidity version upgrade.

The bytecode indicates a Solidity version upgrade from 0.8.25 to 0.8.28. While this is a minor version change, please verify:

  1. The new version is compatible with all dependent contracts
  2. No breaking changes are introduced in the version upgrade
#!/bin/bash
# Description: Check for potential version compatibility issues

# Find all Solidity files and their pragma versions
fd -e sol | xargs rg "pragma solidity"

# Find any version constraints in configuration files
fd "package.json|package-lock.json|yarn.lock" | xargs rg "solc|solidity"

41-41: Verify source contract compilation.

The bytecode update indicates a recompilation of the source Solidity contract. Since this is an auto-generated file, please ensure:

  1. The source contract was compiled with the correct settings
  2. The bindings were generated using the standard toolchain
integration_tests/test_ibc_rly.py (8)

96-96: LGTM: Direct fee value assignment improves transparency

The removal of keccak hashing for the fee value makes the logs more readable and debugging easier.


104-104: LGTM: Direct denomination value improves token tracking

The removal of keccak hashing for the denomination value enhances log readability and token tracking capabilities.


126-130: LGTM: Enhanced packet information improves observability

The changes improve the packet information by:

  1. Using direct sequence values instead of hashes
  2. Adding human-readable source port and channel information
    These improvements make debugging and monitoring IBC packets more effective.

146-150: LGTM: Consistent packet information handling

The changes maintain consistency with the recv_packet function, ensuring uniform packet information handling across different packet types.


159-159: LGTM: Improved denomination tracing

Direct denomination value assignment enhances the clarity of denomination tracing.


165-169: LGTM: Consistent acknowledgment packet handling

The changes align with other packet handling functions, maintaining consistency in how packet information is handled across the codebase.


218-218: LGTM: Proper type handling for packet sequences

Converting packet sequence to integer ensures correct type handling for comparisons and filtering operations.


229-229: Verify handling of large sequence numbers

The 64-character hex format is correct for EVM log topics, but let's verify the handling of large sequence numbers.

CHANGELOG.md (1)

Line range hint 1-1183: LGTM!

The changelog is well-structured with clear categorization of changes and consistent formatting.

🧰 Tools
🪛 Markdownlint

23-23: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go (8)

47-47: ABI updates correctly reflect contract changes

The ABI definition has been updated to include new fields and data types, aligning with the modifications in the smart contract. The changes appear consistent and accurate.


1163-1166: Updated WatchDenominationTrace function

The WatchDenominationTrace function has been updated to reflect the changes in event parameters. Ensure that any subscriptions are updated accordingly.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 1164-1164: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1164
Added line #L1164 was not covered by tests


[warning] 1166-1166: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1166
Added line #L1166 was not covered by tests


Line range hint 1426-1466: Update in RelayerModuleFungibleTokenPacket event handling

The RelayerModuleFungibleTokenPacket struct now includes updated data types for Denom and Amount. The changes align with the ABI updates.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 1434-1434: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1434
Added line #L1434 was not covered by tests


1734-1744: Updates to RelayerModuleRecvPacket event struct

The struct now includes new fields such as PacketSrcPortInfo and PacketSrcChannelInfo. These additions appear consistent with the ABI changes.


Line range hint 1903-1935: Updates to RelayerModuleTimeout event handling

The RefundDenom field type has been changed to string, aligning with the ABI. The updates are appropriate and consistent.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 1911-1911: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1911
Added line #L1911 was not covered by tests


[warning] 1918-1918: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1918
Added line #L1918 was not covered by tests


301-304: Updated WatchAcknowledgePacket function parameters

The parameters of the WatchAcknowledgePacket function have been updated to match the changes in the event definition. Ensure consistency across the codebase.

You can run the following script to verify that all usages are updated:

#!/bin/bash
# Description: Find all usages of WatchAcknowledgePacket to verify parameters.

rg 'WatchAcknowledgePacket\('
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 304-304: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L304
Added line #L304 was not covered by tests


Line range hint 2369-2409: Updates to WriteAcknowledgement event handling

New fields have been added to the RelayerModuleWriteAcknowledgement struct, and function parameters have been updated accordingly. The changes are consistent with the ABI.

Please verify that all usages of these functions are updated:

✅ Verification successful

Let me search with a more comprehensive pattern to find all related usages.


Based on the search results, I can now provide the final response:

WriteAcknowledgement event handling is correctly implemented

The changes to the WriteAcknowledgement event handling are consistent across the codebase:

  • The event is defined in the Solidity contract (Relayer.sol)
  • The Go bindings in i_relayer_module.abigen.go correctly implement all the necessary structs and methods
  • The only usage outside the bindings is in integration_tests/test_ibc_rly.py which only references the event name in a list
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usages of WriteAcknowledgement functions.

rg 'WriteAcknowledgement\('

Length of output: 352


Script:

#!/bin/bash
# Search for any mentions of WriteAcknowledgement in the codebase
rg -i "writeacknowledgement"

Length of output: 14352

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 2384-2384: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L2384
Added line #L2384 was not covered by tests


276-279: Updated FilterAcknowledgePacket function parameters

The parameters of the FilterAcknowledgePacket function have been updated to reflect the new indexed fields in the event. Ensure that all call sites are updated accordingly.

You can run the following script to verify that all call sites are updated:

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 279-279: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L279
Added line #L279 was not covered by tests

Comment on lines +64 to +68
replaceAttrs := map[string]string{
cronoseventstypes.AttributeKeySrcPortInfo: channeltypes.AttributeKeySrcPort,
cronoseventstypes.AttributeKeySrcChannelInfo: channeltypes.AttributeKeySrcChannel,
}
return desc.ConvertEvent(event.Attributes, RelayerValueDecoders, replaceAttrs)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add test coverage for the new attribute replacement logic.

The new attribute replacement logic is not covered by tests. Please add test cases to verify the correct mapping of AttributeKeySrcPortInfo and AttributeKeySrcChannelInfo.

Would you like me to help generate test cases? Here's a suggested test structure:

func TestRelayerConvertEvent_AttributeReplacement(t *testing.T) {
    testCases := []struct {
        name     string
        event    sdk.Event
        expected *ethtypes.Log
    }{
        {
            name: "should replace port info attributes",
            event: sdk.Event{
                Type: "ibc_transfer",
                Attributes: []sdk.Attribute{
                    {Key: cronoseventstypes.AttributeKeySrcPortInfo, Value: "transfer"},
                    {Key: cronoseventstypes.AttributeKeySrcChannelInfo, Value: "channel-0"},
                },
            },
            // Add expected values
        },
        // Add more test cases
    }
    // Implement test logic
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 64-68: x/cronos/events/events.go#L64-L68
Added lines #L64 - L68 were not covered by tests

Comment on lines +265 to +274
PacketSequence *big.Int
PacketSrcPort common.Hash
PacketSrcChannel common.Hash
PacketSrcPortInfo string
PacketSrcChannelInfo string
PacketDstPort string
PacketDstChannel string
ConnectionId string
Raw types.Log // Blockchain specific contextual infos
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider adding tests for the updated RelayerModuleAcknowledgePacket event struct

The RelayerModuleAcknowledgePacket struct has been updated with new fields and data types, but these changes are not currently covered by tests. Adding tests will ensure the correctness and reliability of the new functionality.

Would you like assistance in generating tests for this updated code?

Comment on lines +1145 to +1154
Denom string
Raw types.Log // Blockchain specific contextual infos
}

// FilterDenominationTrace is a free log retrieval operation binding the contract event 0x483180a024351f3ea4c4782eaadb34add715974648a3d47bbff4a7b76da20859.
//
// Solidity: event DenominationTrace(string indexed denom)
func (_RelayerModule *RelayerModuleFilterer) FilterDenominationTrace(opts *bind.FilterOpts, denom []string) (*RelayerModuleDenominationTraceIterator, error) {
// Solidity: event DenominationTrace(string denom)
func (_RelayerModule *RelayerModuleFilterer) FilterDenominationTrace(opts *bind.FilterOpts) (*RelayerModuleDenominationTraceIterator, error) {

var denomRule []interface{}
for _, denomItem := range denom {
denomRule = append(denomRule, denomItem)
}

logs, sub, err := _RelayerModule.contract.FilterLogs(opts, "DenominationTrace", denomRule)
logs, sub, err := _RelayerModule.contract.FilterLogs(opts, "DenominationTrace")
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider adding tests for the updated DenominationTrace event handling

The RelayerModuleDenominationTrace struct and associated functions have been updated but are not currently covered by tests. Adding tests will enhance reliability.

Would you like assistance in generating tests for this code?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 1152-1152: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1152
Added line #L1152 was not covered by tests


[warning] 1154-1154: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1154
Added line #L1154 was not covered by tests

Comment on lines +1280 to +1294
Fee string
Raw types.Log // Blockchain specific contextual infos
}

// FilterDistributeFee is a free log retrieval operation binding the contract event 0x67e2bceb7881996b4bbddf9ab5d5c9bceb0ace3a06538b5e40be96094c4c9a72.
//
// Solidity: event DistributeFee(address indexed receiver, string indexed fee)
func (_RelayerModule *RelayerModuleFilterer) FilterDistributeFee(opts *bind.FilterOpts, receiver []common.Address, fee []string) (*RelayerModuleDistributeFeeIterator, error) {
// Solidity: event DistributeFee(address indexed receiver, string fee)
func (_RelayerModule *RelayerModuleFilterer) FilterDistributeFee(opts *bind.FilterOpts, receiver []common.Address) (*RelayerModuleDistributeFeeIterator, error) {

var receiverRule []interface{}
for _, receiverItem := range receiver {
receiverRule = append(receiverRule, receiverItem)
}
var feeRule []interface{}
for _, feeItem := range fee {
feeRule = append(feeRule, feeItem)
}

logs, sub, err := _RelayerModule.contract.FilterLogs(opts, "DistributeFee", receiverRule, feeRule)
logs, sub, err := _RelayerModule.contract.FilterLogs(opts, "DistributeFee", receiverRule)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider adding tests for the updated DistributeFee event handling

The RelayerModuleDistributeFee struct and associated functions have been updated but are not currently covered by tests. Adding tests will ensure the correctness of the new functionality.

Would you like assistance in generating tests for this code?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 1287-1287: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1287
Added line #L1287 was not covered by tests


[warning] 1294-1294: x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_module.abigen.go#L1294
Added line #L1294 was not covered by tests

Merged via the queue into crypto-org-chain:main with commit 1ca4595 Oct 23, 2024
37 of 38 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