feat: IBC packet tracking adjustments #748
Conversation
WalkthroughThis pull request introduces several updates across the Andromeda protocol's contracts, focusing on improvements in the splitter contract, kernel execution, and IBC packet handling. The changes include version bumps for the andromeda-splitter and andromeda-kernel packages, modifications to the splitter's coin distribution logic, enhanced error handling in the kernel's execute functions, and updates to test cases for cross-chain recipient functionality. Changes
Sequence DiagramsequenceDiagram
participant Sender
participant Splitter
participant Recipients
Sender->>Splitter: Send funds
Splitter->>Splitter: Calculate recipient shares
Splitter->>Recipients: Distribute funds
Splitter-->>Sender: Confirm distribution
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests-integration/tests/splitter.rs (1)
617-637: Validate IBC Recipient Strings for SafetyThe cross-chain recipients use strings like
"ibc://osmosis/...". Consider adding validation checks or a parsing routine to ensure the chain IDs and addresses adhere to your conventional format, avoiding potential injection or encoding issues.contracts/os/andromeda-kernel/src/execute.rs (2)
47-47: Consider More Descriptive Error HandlingUsing
.expect("No packet found for channel_id and sequence")triggers a panic. Replacing it with a customContractErroror returning an error would offer a cleaner failure path and consistent error handling.- .expect("No packet found for channel_id and sequence"); + .map_err(|_| ContractError::InvalidPacket { + error: Some("No packet found for the specified channel_id and sequence".to_string()), + })?
215-219: Skipping the First MessageExplicitly skipping the first message is a practical way to differentiate it from the rest. Consider adding logs or commentary clarifying that the first message has already been processed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)contracts/finance/andromeda-splitter/Cargo.toml(1 hunks)contracts/finance/andromeda-splitter/src/contract.rs(1 hunks)contracts/os/andromeda-kernel/Cargo.toml(1 hunks)contracts/os/andromeda-kernel/src/execute.rs(8 hunks)tests-integration/tests/splitter.rs(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- contracts/os/andromeda-kernel/Cargo.toml
- contracts/finance/andromeda-splitter/Cargo.toml
🔇 Additional comments (13)
tests-integration/tests/splitter.rs (2)
654-654: Increased Funding for SplitterRaising the split amount from 100 to 200 is acceptable. Ensure you have additional tests covering smaller or zero split amounts for edge-case scenarios.
674-674: Confirm Sufficient Post-Send BalanceThe assertion verifying the entire 200 tokens reached the kernel is correct. If partial distributions are expected in other tests, consider adding them to ensure distribution logic remains robust.
contracts/finance/andromeda-splitter/src/contract.rs (1)
209-214: Streamlined Distribution LogicAppending directly to
amp_fundsand immediately generating AMP messages simplifies the logic. Ensure any potential overflow or large coin calculations are still safe within these loops, and consider the maximum possible number of recipients to mitigate DoS concerns.contracts/os/andromeda-kernel/src/execute.rs (9)
8-8: Good Use of Merged Coin UtilitiesBringing in
has_coins_mergedandmerge_coinshelps centralize coin-fund checks. This should reduce duplication and potential errors across code paths.
71-71: Maintaining Packet SequencePassing
packet_sequenceforward in the error handling is clear and consistent with the operation. Consider logging or storing the sequence in more contexts to ease debugging if an issue arises.
83-87: Refund Attributes Provide ClarityAdding these attributes for a refund outcome boosts observability, allowing off-chain systems to parse logs for debug or analytics. This is a helpful pattern for multi-chain event tracking.
163-168: Enhanced Relay Success MetadataThe additional attributes for a successful relay highlight the chain ID, channel, and receiving kernel. This is an excellent improvement in cross-chain traceability.
179-180: Strengthened Access Control on AMP ReceptionVerifying that
info.sender == env.contract.address || query::verify_address(...)?.verify_addressprevents unauthorized calls. Good to see robust permission checks aligned with the contract’s security model.
198-207: Handling the First AMP Message ExplicitlyProcessing only the first message separately can reduce complexity but be sure to handle all edge cases, like packets containing only one message versus multiple. The code is clear as written.
208-214: Message Aggregation for Additional AMP MessagesAppending the handled messages into
resand buildingnew_pktfor the remainder is a neat approach. Validate that large or invalid packets do not degrade performance or lead to memory issues.
221-230: Conditional Sub-packet CreationTriggering a new sub-message only when subsequent AMP messages exist is efficient. If an external caller attempts to craft packets with extraneous data, it will be gracefully discarded.
856-859: Single Coin Restriction for ICS20 TransfersLimiting
funds.len()to exactly one in ICS20 ensures a simpler flow. Consider documenting or validating corner cases where bridging multiple coins might be desired in the future.CHANGELOG.md (1)
30-30: Clear Documentation of IBC Packet Tracking FeatureReferencing PR #748 in the changelog is excellent. It helps maintainers trace changes. Make sure to include any known migration instructions or backward compatibility notes, if applicable.
Motivation
These changes are to aid in off chain tracking of packets. This includes:
Testing
The splitter test for IBC was updated to reflect the fixes from above
Version Changes
kernel:1.2.0-b.3->1.2.0-b.4splitter:2.3.0-b.1->2.3.0-b.2Summary by CodeRabbit
Release Notes
New Features
Changes
Bug Fixes
Performance