feat: Failed Ack Handling #582
Conversation
…ents and extra checks to integration test
…Data struct, rename reply function
WalkthroughThe pull request involves updates to multiple Changes
Possibly related PRs
Suggested reviewers
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: 16
🧹 Outside diff range and nitpick comments (6)
packages/std/src/common/reply.rs (1)
13-13: LGTM! Consider adding a brief comment for clarity.The addition of the
IBCTransferWithMsgvariant is consistent with the existing enum structure and aligns with the PR objectives for handling IBC transfers with additional message data.Consider adding a brief comment to explain the purpose of this new variant, similar to the comments for other sections (e.g., "// Kernel", "// App"). This would improve code readability and maintainability.
IBCTransfer = 106, + // IBC transfer with additional message data IBCTransferWithMsg = 107,contracts/os/andromeda-kernel/Cargo.toml (1)
48-48: Consider documenting the relationship between cw-orch update and new features.While this Cargo.toml update doesn't directly implement the failed acknowledgment handling and refund features mentioned in the PR objectives, it's possible that the new version of cw-orch (0.24.1) provides functionality that supports these features.
Consider adding a comment in the Cargo.toml file or updating the changelog to explain why this update was necessary and how it relates to the new features being implemented. This will help future developers understand the context of this change.
contracts/os/andromeda-kernel/src/state.rs (1)
48-49: LGTM: RefundData constant is well-defined.The
REFUND_DATAconstant is appropriately defined and aligns with the PR's objective of managing refund information. Its placement and structure are consistent with other constants in the file.Consider adding a brief comment explaining the purpose of
REFUND_DATA, similar to other constants in the file. For example:/// Used to store refund information for failed ICS20 transfers pub const REFUND_DATA: Item<RefundData> = Item::new("refund_data");contracts/os/andromeda-kernel/src/reply.rs (1)
Line range hint
97-168: UseReplyIdenum instead of hardcoded reply IDIn the
on_reply_ibc_transferfunction, the reply ID is hardcoded as106. For consistency and maintainability, consider defining a corresponding variant in theReplyIdenum and using it instead.Apply this change:
-if let Reply { - id: 106, +if let Reply { + id: ReplyId::IBCTransfer as u64,Ensure that
IBCTransferis defined in theReplyIdenum:pub enum ReplyId { UpdateOwnership = 0, // other variants... IBCTransfer = 106, }contracts/os/andromeda-kernel/src/execute.rs (1)
751-756: Consider setting agas_limitfor the submessageCurrently,
gas_limitis set toNonewhen adding the submessage. Depending on the message size and complexity, this might lead to unexpected gas consumption. Setting an appropriategas_limithelps in controlling gas usage.Example:
resp = resp.add_submessage(SubMsg { id: ReplyId::IBCTransfer.repr(), msg: CosmosMsg::Ibc(msg), gas_limit: Some(200_000), // Adjust the gas limit as needed reply_on: cosmwasm_std::ReplyOn::Always, });tests-integration/tests/kernel_orch.rs (1)
1381-1454: Add assertions to validate refund upon execution failureAfter simulating a failure in the execution message, the test checks the sender's balance to ensure a refund occurred. To strengthen the test, consider adding explicit assertions on the kernel's state and any relevant events or logs to confirm that the refund was processed correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
- contracts/data-storage/andromeda-counter/Cargo.toml (1 hunks)
- contracts/finance/andromeda-splitter/Cargo.toml (1 hunks)
- contracts/fungible-tokens/andromeda-ics20/Cargo.toml (1 hunks)
- contracts/os/andromeda-adodb/Cargo.toml (1 hunks)
- contracts/os/andromeda-economics/Cargo.toml (1 hunks)
- contracts/os/andromeda-kernel/Cargo.toml (1 hunks)
- contracts/os/andromeda-kernel/src/contract.rs (3 hunks)
- contracts/os/andromeda-kernel/src/execute.rs (6 hunks)
- contracts/os/andromeda-kernel/src/ibc.rs (4 hunks)
- contracts/os/andromeda-kernel/src/interface.rs (0 hunks)
- contracts/os/andromeda-kernel/src/reply.rs (4 hunks)
- contracts/os/andromeda-kernel/src/state.rs (2 hunks)
- contracts/os/andromeda-vfs/Cargo.toml (1 hunks)
- packages/std/src/common/reply.rs (1 hunks)
- packages/std/src/os/kernel.rs (4 hunks)
- tests-integration/Cargo.toml (1 hunks)
- tests-integration/tests/kernel_orch.rs (10 hunks)
💤 Files with no reviewable changes (1)
- contracts/os/andromeda-kernel/src/interface.rs
🧰 Additional context used
🔇 Additional comments (22)
contracts/finance/andromeda-splitter/Cargo.toml (1)
23-23: LGTM: Dependency update looks good.The update of cw-orch from 0.23.0 to 0.24.1 is consistent with changes in other packages. This is likely part of a project-wide update.
To ensure this update doesn't introduce any breaking changes, please run the following verification script:
Also, don't forget to update the changelog if you haven't already done so.
contracts/os/andromeda-adodb/Cargo.toml (1)
31-31: LGTM: cw-orch dependency updated.The update of the cw-orch dependency from 0.23.0 to 0.24.1 is consistent with similar updates across other packages in the project. This is a good practice for maintaining up-to-date dependencies.
To ensure this update doesn't introduce any breaking changes, please run the following verification script:
If any issues are found, please make the necessary adjustments to the code to accommodate the new version.
contracts/os/andromeda-vfs/Cargo.toml (1)
31-31: Dependency version update looks good, but verify compatibility.The
cw-orchdependency has been updated from version 0.23.0 to 0.24.1, which is consistent with the changes mentioned in the AI-generated summary for other packages in the project.To ensure consistency across the project and identify any potential compatibility issues, please run the following script:
Please review the script output to ensure all packages have been updated consistently and to identify any breaking changes that may require additional modifications in the codebase.
contracts/data-storage/andromeda-counter/Cargo.toml (1)
32-32: Dependency version update looks good.The
cw-orchdependency has been updated from version 0.23.0 to 0.24.1. This change is consistent with the updates made across other packages in this pull request.To ensure consistency across the project, let's verify that this update has been applied to all relevant Cargo.toml files:
✅ Verification successful
Dependency version consistency verified.
All
Cargo.tomlfiles have updated thecw-orchdependency to version0.24.1, and no instances of the old version0.23.0were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify cw-orch version consistency across all Cargo.toml files # Test: Search for cw-orch dependency in all Cargo.toml files # Expect: All occurrences should show version 0.24.1 rg 'cw-orch.*=.*0\.24\.1' -g 'Cargo.toml' # Test: Check for any remaining occurrences of the old version # Expect: No results rg 'cw-orch.*=.*0\.23\.0' -g 'Cargo.toml'Length of output: 689
contracts/os/andromeda-kernel/Cargo.toml (1)
48-48: Dependency update looks good, but verify compatibility.The update of
cw-orchfrom version 0.23.0 to 0.24.1 is consistent with the changes mentioned in the AI-generated summary. This update likely brings improvements or bug fixes to the cw-orch library.To ensure this update doesn't introduce any breaking changes or compatibility issues, please run the following verification steps:
If any issues are found during these checks, please address them before merging this PR.
contracts/os/andromeda-kernel/src/state.rs (2)
1-1: LGTM: Import of RefundData is appropriate.The addition of
RefundDatato the imports is consistent with the PR's objective of implementing failed acknowledgment handling and refunds in the ICS20 transfer process.
Line range hint
1-49: Overall changes are well-integrated and minimal.The additions to this file are focused and align well with the existing structure. They enhance the state management capabilities for handling refunds without disrupting the current functionality. The changes are consistent with the PR objectives and maintain good code organization.
tests-integration/Cargo.toml (1)
133-134: Dependency updates look good, verify compatibility.The updates to
cw-orch(0.24.1) andcw-orch-interchain(0.3.0) align with the project-wide updates mentioned in the summary. These changes likely support the new features for handling failed acknowledgments in ICS20 transfers.To ensure these updates don't introduce any breaking changes, please run the following command:
If any issues arise, please review the changelog for these dependencies to identify any breaking changes that might need to be addressed in the codebase.
packages/std/src/os/kernel.rs (2)
176-197: Implementation ofAcknowledgementMsgand its methods is soundThe
AcknowledgementMsgenum and itsunwrapandunwrap_errmethods are properly implemented, handling acknowledgment messages effectively.
173-174: Definition ofSendMessageWithFundsResponseis appropriateThe empty struct
SendMessageWithFundsResponseis correctly defined and can be used for future response data if needed.contracts/os/andromeda-kernel/src/contract.rs (4)
14-16: Addition ofon_reply_refund_ibc_transfer_with_msgImportThe import of
on_reply_refund_ibc_transfer_with_msgis appropriate and necessary for handling refund scenarios in IBC transfers with messages.
50-50: MadedepsMutable inreplyFunctionChanging
depsto mutable indicates that state modifications are now performed within thereplyfunction. Ensure that this change is intentional and that all state changes are correctly managed.
52-67: Enhanced Error Handling forIBCTransferWithMsgThe addition of specific error handling for
ReplyId::IBCTransferWithMsgimproves the robustness of thereplyfunction by appropriately managing refund scenarios when an IBC transfer with a message fails.
100-103: UpdatedTriggerRelayto Includepack_ack_msgParameterThe inclusion of the
pack_ack_msgparameter in theExecuteMsg::TriggerRelayvariant enhances the functionality by allowing more detailed handling of relay triggers, particularly for IBC acknowledgment messages.contracts/os/andromeda-kernel/src/reply.rs (2)
2-6: LGTM!The imported modules are appropriate and necessary for the implemented functionalities.
17-17: LGTM!The additions to the imports from
cosmwasm_stdare appropriate.contracts/os/andromeda-kernel/src/execute.rs (4)
11-14: Imported modules and types are correctly includedThe newly imported modules and types are necessary for the enhanced IBC message handling and acknowledgment processing.
Also applies to: 17-19
80-90: Ensure the correctness of refund logic in case of acknowledgment errorsWhen handling
AcknowledgementMsg::Error, the code constructs a refund message to the original sender. Verify that the refund amount and recipient are correctly determined to prevent any fund mismanagement.Confirm that
ics20_packet_info.senderandics20_packet_info.fundsaccurately represent the original sender and the exact amount initially sent.
727-731: Validation of funds is handled correctlyThe code properly checks that
fundsis not empty before proceeding, returning an appropriate error if it is. This prevents potential runtime errors due to accessing an empty collection.
43-43: Verify all calls totrigger_relayinclude the newpack_ack_msgparameterThe function
trigger_relaynow includes a new parameterpack_ack_msg: IbcPacketAckMsg. Please ensure that all invocations of this function have been updated accordingly to prevent any compile-time errors or unintended behavior.You can run the following script to find any calls to
trigger_relaythat may not include the new parameter:✅ Verification successful
All calls to
trigger_relayinclude the newpack_ack_msgparameter🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find calls to `trigger_relay` that may not have been updated rg 'trigger_relay\s*\(' -g '*.rs' -A 1Length of output: 410
tests-integration/tests/kernel_orch.rs (2)
1033-1034: Potential timing issue with chain advancementThe call to
osmosis.wait_seconds(604_810).unwrap();advances the chain time by a significant duration. Ensure that this large time jump does not affect the outcome of the test inadvertently or impact subsequent tests due to shared state in the test environment.
1040-1102:⚠️ Potential issueEnsure correct handling of failed packet outcomes
In the else block starting at line 1042, the test assumes the packet has failed and proceeds to manually trigger the relay with an error acknowledgment. Verify that this simulates the intended failure scenario accurately and that the refund logic is properly tested. Additionally, check that the final balance assertions confirm the sender has been refunded as expected.
| @@ -31,7 +31,7 @@ andromeda-std = { workspace = true } | |||
|
|
|||
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | |||
| cw-multi-test = { workspace = true, optional = true } | |||
| cw-orch = "=0.23.0" | |||
| cw-orch = "=0.24.1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent cw-orch Dependency Versions Detected.
Only 2 out of 49 Cargo.toml files are using cw-orch = "0.24.1". Please update the following files to ensure consistency across the project:
tests-integration/Cargo.tomlcontracts/os/andromeda-economics/Cargo.toml
🔗 Analysis chain
Dependency update looks good, but let's verify consistency.
The update of cw-orch from version 0.23.0 to 0.24.1 is in line with the changes observed in other packages. This update likely brings improvements or fixes that benefit the project.
To ensure this update is intentional and consistent across the project, please run the following script:
This script will help us confirm that the version update has been applied consistently across all relevant Cargo.toml files in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify cw-orch version consistency across all Cargo.toml files
# Test: Check for any Cargo.toml files that don't use cw-orch version 0.24.1
# Expect: No output if all files are consistent
rg -U 'cw-orch.*=.*(?!0\.24\.1)' $(fd Cargo.toml)
# Test: Count occurrences of cw-orch version 0.24.1
# Expect: A number equal to the total number of Cargo.toml files that use cw-orch
echo "Number of Cargo.toml files using cw-orch 0.24.1:"
rg -c 'cw-orch.*=.*0\.24\.1' $(fd Cargo.toml) | awk -F: '{sum += $2} END {print sum}'
Length of output: 5169
Script:
#!/bin/bash
# Description: Verify cw-orch version consistency across all Cargo.toml files
# Test: Check for any Cargo.toml files that don't use cw-orch version 0.24.1
# Using --pcre2 to enable look-around in regex
rg --pcre2 -U 'cw-orch\s*=\s*"(?!0\.24\.1)' $(fd Cargo.toml)
# Test: Count occurrences of cw-orch version 0.24.1
echo "Number of Cargo.toml files using cw-orch 0.24.1:"
rg -c 'cw-orch\s*=\s*"0\.24\.1"' $(fd Cargo.toml) | awk -F: '{sum += $2} END {print sum}'
# Test: Count total number of Cargo.toml files
echo "Total number of Cargo.toml files:"
fd Cargo.toml | wc -l
Length of output: 5552
| @@ -28,7 +28,7 @@ cw-controllers = "=1.1.2" | |||
| schemars = "0.8.21" | |||
| serde = "1.0.203" | |||
| thiserror = "1.0.61" | |||
| cw-orch = "=0.23.0" | |||
| cw-orch = "=0.24.1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent cw-orch Dependency Versions Detected
The cw-orch dependency is not updated to version =0.24.1 in the following Cargo.toml files:
- contracts/app/andromeda-app-contract/Cargo.toml
- contracts/data-storage/andromeda-boolean/Cargo.toml
- contracts/data-storage/andromeda-primitive/Cargo.toml
- contracts/data-storage/andromeda-string-storage/Cargo.toml
- contracts/ecosystem/andromeda-vault/Cargo.toml
- contracts/finance/andromeda-conditional-splitter/Cargo.toml
- contracts/finance/andromeda-cross-chain-swap/Cargo.toml
- contracts/finance/andromeda-rate-limiting-withdrawals/Cargo.toml
- contracts/finance/andromeda-set-amount-splitter/Cargo.toml
- contracts/finance/andromeda-timelock/Cargo.toml
- contracts/finance/andromeda-validator-staking/Cargo.toml
- contracts/finance/andromeda-vesting/Cargo.toml
- contracts/finance/andromeda-weighted-distribution-splitter/Cargo.toml
- contracts/fungible-tokens/andromeda-cw20/Cargo.toml
- contracts/fungible-tokens/andromeda-cw20-exchange/Cargo.toml
- contracts/fungible-tokens/andromeda-cw20-staking/Cargo.toml
- contracts/fungible-tokens/andromeda-lockdrop/Cargo.toml
- contracts/fungible-tokens/andromeda-merkle-airdrop/Cargo.toml
- contracts/modules/andromeda-address-list/Cargo.toml
- contracts/modules/andromeda-curve/Cargo.toml
- contracts/modules/andromeda-date-time/Cargo.toml
- contracts/modules/andromeda-rates/Cargo.toml
- contracts/modules/andromeda-shunting/Cargo.toml
- contracts/non-fungible-tokens/andromeda-auction/Cargo.toml
- contracts/non-fungible-tokens/andromeda-crowdfund/Cargo.toml
- contracts/non-fungible-tokens/andromeda-cw721/Cargo.toml
- contracts/non-fungible-tokens/andromeda-marketplace/Cargo.toml
- contracts/os/andromeda-ibc-registry/Cargo.toml
- ibc-tests/Cargo.toml
- packages/andromeda-app/Cargo.toml
- packages/andromeda-data-storage/Cargo.toml
- packages/andromeda-ecosystem/Cargo.toml
- packages/andromeda-finance/Cargo.toml
- packages/andromeda-fungible-tokens/Cargo.toml
- packages/andromeda-modules/Cargo.toml
- packages/andromeda-non-fungible-tokens/Cargo.toml
- packages/andromeda-testing/Cargo.toml
- packages/andromeda-testing-e2e/Cargo.toml
- packages/std/Cargo.toml
- packages/std/macros/Cargo.toml
- tests-integration/Cargo.toml
🔗 Analysis chain
Dependency version update looks good.
The cw-orch dependency has been updated from version 0.23.0 to 0.24.1. This change is consistent with updates made across multiple packages in the project, as mentioned in the AI-generated summary.
To ensure consistency across the project, let's verify this update in other packages:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify cw-orch version consistency across packages
# Test: Check cw-orch version in all Cargo.toml files
rg -U 'cw-orch.*=.*0\.24\.1' '**/Cargo.toml'
Length of output: 100
Script:
#!/bin/bash
# Description: Locate all Cargo.toml files and verify cw-orch version
# Find all Cargo.toml files
fd Cargo.toml > cargo_files.txt
# Check cw-orch version in each Cargo.toml
while IFS= read -r file; do
echo "Checking $file"
grep 'cw-orch\s*=\s*"\=0\.24\.1"' "$file"
done < cargo_files.txt
Length of output: 11382
Script:
#!/bin/bash
# Description: List all Cargo.toml files that do not have cw-orch version "=0.24.1"
# Find all Cargo.toml files
fd Cargo.toml > cargo_files.txt
# List files without cw-orch = "=0.24.1"
while IFS= read -r file; do
if ! grep -q 'cw-orch\s*=\s*"\=0\.24\.1"' "$file"; then
echo "$file"
fi
done < cargo_files.txt
Length of output: 9643
| pub original_sender: String, | ||
| pub funds: Coin, | ||
| pub channel: String, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use AndrAddr instead of String for original_sender in RefundData
To maintain consistency and ensure type safety across your codebase, consider changing the type of original_sender to AndrAddr.
Apply this diff to update the struct:
pub struct RefundData {
- pub original_sender: String,
+ pub original_sender: AndrAddr,
pub funds: Coin,
pub channel: String,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub original_sender: String, | |
| pub funds: Coin, | |
| pub channel: String, | |
| } | |
| pub struct RefundData { | |
| pub original_sender: AndrAddr, | |
| pub funds: Coin, | |
| pub channel: String, | |
| } |
| @@ -135,6 +138,7 @@ | |||
| recipient: AndrAddr, | |||
| message: Binary, | |||
| funds: Coin, | |||
| original_sender: String, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use AndrAddr instead of String for original_sender
For consistency and type safety, consider using AndrAddr instead of String for the original_sender field.
Apply this diff to update the type:
SendMessageWithFunds {
recipient: AndrAddr,
message: Binary,
funds: Coin,
- original_sender: String,
+ original_sender: AndrAddr,
},Committable suggestion was skipped due to low confidence.
| let ack: AcknowledgementMsg<SendMessageWithFundsResponse> = | ||
| from_json(pack_ack_msg.acknowledgement.data)?; | ||
|
|
There was a problem hiding this comment.
Handle potential deserialization errors of the acknowledgment message
Deserializing the acknowledgment message using from_json could fail if the data is malformed. Consider adding error handling to catch deserialization errors and respond appropriately.
Modify the code to handle deserialization errors:
let ack_result = from_json(pack_ack_msg.acknowledgement.data);
let ack: AcknowledgementMsg<SendMessageWithFundsResponse> = match ack_result {
Ok(a) => a,
Err(e) => {
return Err(ContractError::Std(StdError::generic_err(format!("Failed to parse acknowledgment: {}", e))));
}
};| PENDING_MSG_AND_FUNDS.save( | ||
| deps.storage, | ||
| &Ics20PacketInfo { | ||
| sender: info.sender.into_string(), | ||
| recipient: recipient.clone(), | ||
| message: message.clone(), | ||
| funds: coin, | ||
| channel: channel.clone(), | ||
| }, | ||
| )?; |
There was a problem hiding this comment.
Prevent potential data overwriting in PENDING_MSG_AND_FUNDS storage
Storing Ics20PacketInfo without a unique key could lead to data overwriting if multiple transfers occur simultaneously from the same sender. Ensure that the storage key is unique per transaction.
Consider using a combination of channel and packet_sequence as the storage key:
let storage_key = format!("{}:{}", channel.clone(), packet_sequence);
PENDING_MSG_AND_FUNDS.save(
deps.storage,
&storage_key,
&Ics20PacketInfo {
sender: info.sender.into_string(),
recipient: recipient.clone(),
message: message.clone(),
funds: coin,
channel: channel.clone(),
},
)?;| let kernel_juno_send_request = kernel_juno | ||
| .execute( | ||
| &ExecuteMsg::Send { | ||
| message: AMPMsg { | ||
| recipient: AndrAddr::from_string(format!( | ||
| "ibc://osmosis/{}", | ||
| kernel_osmosis.address().unwrap() | ||
| )), | ||
| message: Binary::default(), | ||
| funds: vec![Coin { | ||
| denom: "juno".to_string(), | ||
| amount: Uint128::new(100), | ||
| }], | ||
| config: AMPMsgConfig { | ||
| reply_on: cosmwasm_std::ReplyOn::Always, | ||
| exit_at_error: false, | ||
| gas_limit: None, | ||
| direct: true, | ||
| ibc_config: None, | ||
| }, | ||
| }, | ||
| }, | ||
| Some(&[Coin { | ||
| denom: "juno".to_string(), | ||
| amount: Uint128::new(100), | ||
| }]), | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| osmosis.wait_seconds(604_810).unwrap(); | ||
|
|
||
| let packet_lifetime = interchain | ||
| .await_packets("juno", kernel_juno_send_request) | ||
| .unwrap(); | ||
|
|
||
| // For testing a successful outcome of the first packet sent out in the tx, you can use: | ||
| if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome { | ||
| // Packet has been successfully acknowledged and decoded, the transaction has gone through correctly | ||
| } else { | ||
| // Register trigger address | ||
| kernel_juno | ||
| .execute( | ||
| &ExecuteMsg::UpsertKeyAddress { | ||
| key: "trigger_key".to_string(), | ||
| value: sender.clone(), | ||
| }, | ||
| None, | ||
| ) | ||
| .unwrap(); | ||
| let kernel_juno_splitter_request = kernel_juno | ||
| .execute( | ||
| &ExecuteMsg::TriggerRelay { | ||
| packet_sequence: "1".to_string(), | ||
| pack_ack_msg: IbcPacketAckMsg::new( | ||
| IbcAcknowledgement::new( | ||
| to_json_binary( | ||
| &AcknowledgementMsg::<SendMessageWithFundsResponse>::Error( | ||
| "error".to_string(), | ||
| ), | ||
| ) | ||
| .unwrap(), | ||
| ), | ||
| IbcPacket::new( | ||
| Binary::default(), | ||
| IbcEndpoint { | ||
| port_id: "port_id".to_string(), | ||
| channel_id: "channel_id".to_string(), | ||
| }, | ||
| IbcEndpoint { | ||
| port_id: "port_id".to_string(), | ||
| channel_id: "channel_id".to_string(), | ||
| }, | ||
| 1, | ||
| IbcTimeout::with_timestamp(Timestamp::from_seconds(1)), | ||
| ), | ||
| Addr::unchecked("relayer"), | ||
| ), | ||
| }, | ||
| None, | ||
| ) | ||
| .unwrap(); | ||
| let _packet_lifetime = interchain | ||
| .await_packets("juno", kernel_juno_splitter_request) | ||
| .unwrap(); | ||
|
|
||
| let balances = juno.query_all_balances(sender).unwrap(); | ||
| assert_eq!(balances.len(), 1); | ||
| assert_eq!(balances[0].denom, "juno"); | ||
| // Original amount | ||
| assert_eq!(balances[0].amount, Uint128::new(100000000000000)); | ||
|
|
||
| // Make sure kernel has no funds | ||
| let balances = juno | ||
| .query_all_balances(kernel_juno.address().unwrap()) | ||
| .unwrap(); | ||
| assert_eq!(balances.len(), 0); | ||
| // There was a decode error or the packet timed out | ||
| // Else the packet timed-out, you may have a relayer error or something is wrong in your application | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated setup code into a helper function
The test function test_kernel_ibc_funds_only_unhappy contains setup code that is nearly identical to other test functions in the file. To improve maintainability and reduce code duplication, consider abstracting the common setup logic into a reusable helper function or fixture.
| }, | ||
| }, | ||
| }, | ||
| Some(&[Coin { | ||
| denom: "juno".to_string(), | ||
| amount: Uint128::new(100), | ||
| }]), | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let packet_lifetime = interchain | ||
| .await_packets("juno", kernel_juno_send_request) | ||
| .unwrap(); | ||
| // Make sure the sender's balance decreased by 100 | ||
| let balances = juno.query_all_balances(sender.clone()).unwrap(); | ||
| assert_eq!(balances.len(), 1); | ||
| assert_eq!(balances[0].denom, "juno"); | ||
| // Original amount | ||
| assert_eq!(balances[0].amount, Uint128::new(100000000000000 - 100)); | ||
|
|
||
| // For testing a successful outcome of the first packet sent out in the tx, you can use: | ||
| if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome { | ||
| // Register trigger address | ||
| kernel_juno | ||
| .execute( | ||
| &ExecuteMsg::UpsertKeyAddress { | ||
| key: "trigger_key".to_string(), | ||
| value: sender.clone(), | ||
| }, | ||
| None, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| // Construct an Execute msg from the kernel on juno inteded for the splitter on osmosis | ||
| let kernel_juno_splitter_request = kernel_juno | ||
| .execute( | ||
| &ExecuteMsg::TriggerRelay { | ||
| packet_sequence: "1".to_string(), | ||
| pack_ack_msg: IbcPacketAckMsg::new( | ||
| IbcAcknowledgement::new( | ||
| to_json_binary( | ||
| // It's Ok because the ics20 transfer is supposed to go through. We want the ExecuteMsg to fail | ||
| &AcknowledgementMsg::<SendMessageWithFundsResponse>::Ok( | ||
| SendMessageWithFundsResponse {}, | ||
| ), | ||
| ) | ||
| .unwrap(), | ||
| ), | ||
| IbcPacket::new( | ||
| Binary::default(), | ||
| IbcEndpoint { | ||
| port_id: "port_id".to_string(), | ||
| channel_id: "channel_id".to_string(), | ||
| }, | ||
| IbcEndpoint { | ||
| port_id: "port_id".to_string(), | ||
| channel_id: "channel_id".to_string(), | ||
| }, | ||
| 1, | ||
| IbcTimeout::with_timestamp(Timestamp::from_seconds(1)), | ||
| ), | ||
| Addr::unchecked("relayer"), | ||
| ), | ||
| }, | ||
| None, | ||
| ) | ||
| .unwrap(); | ||
| // We call UpadeLock, a Msg that doesn't accept funds. So it will error and should trigger a refund from the destination chain | ||
| interchain | ||
| .await_and_check_packets("juno", kernel_juno_splitter_request.clone()) | ||
| .unwrap(); | ||
|
|
||
| // Make sure kernel has no funds | ||
| let balances = juno | ||
| .query_all_balances(kernel_juno.address().unwrap()) | ||
| .unwrap(); | ||
| assert_eq!(balances.len(), 0); | ||
|
|
||
| let balances = juno.query_all_balances(sender).unwrap(); | ||
| assert_eq!(balances.len(), 1); | ||
| assert_eq!(balances[0].denom, "juno"); | ||
| // Original amount | ||
| assert_eq!(balances[0].amount, Uint128::new(100000000000000)); | ||
|
|
||
| // // For testing a successful outcome of the first packet sent out in the tx, you can use: | ||
| // if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome { | ||
| // // Packet has been successfully acknowledged and decoded, the transaction has gone through correctly | ||
| // } else { | ||
| // panic!("packet timed out"); | ||
| // // There was a decode error or the packet timed out | ||
| // // Else the packet timed-out, you may have a relayer error or something is wrong in your application | ||
| // }; | ||
|
|
||
| // Packet has been successfully acknowledged and decoded, the transaction has gone through correctly | ||
| } else { | ||
| panic!("packet timed out"); | ||
| // There was a decode error or the packet timed out | ||
| // Else the packet timed-out, you may have a relayer error or something is wrong in your application | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in test_kernel_ibc_funds_and_execute_msg_unhappy
The function test_kernel_ibc_funds_and_execute_msg_unhappy repeats much of the setup code found in other tests. Abstracting the common initialization steps into helper functions will enhance code readability and simplify future maintenance.
| // It's Ok because the ics20 transfer is supposed to go through. We want the ExecuteMsg to fail | ||
| &AcknowledgementMsg::<SendMessageWithFundsResponse>::Ok( | ||
| SendMessageWithFundsResponse {}, | ||
| ), |
There was a problem hiding this comment.
Clarify the use of AcknowledgementMsg::Ok in error scenario
At lines 1412 to 1415, the acknowledgment message is constructed as an AcknowledgementMsg::Ok(SendMessageWithFundsResponse {}), even though the execution is expected to fail. Ensure this accurately reflects the scenario where the ICS20 transfer succeeds, but the subsequent message execution fails, triggering the refund logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
contracts/os/andromeda-kernel/src/execute.rs (2)
Line range hint
41-91: LGTM: Improved IBC acknowledgment handling intrigger_relay.The changes implement the handling of failed acknowledgments as described in the PR objectives. The function now correctly processes both successful and error cases.
Consider adding error handling for the deserialization of the acknowledgment message:
let ack: AcknowledgementMsg<SendMessageWithFundsResponse> = from_json(packet_ack_msg.acknowledgement.data) .map_err(|e| ContractError::Std(StdError::generic_err(format!("Failed to parse acknowledgment: {}", e))))?;This will provide more detailed error information if the deserialization fails.
Line range hint
727-756: LGTM: Improved IBC transfer handling with packet info storage.The changes correctly implement the storage of
Ics20PacketInfoand send the IBC transfer as a submessage. This allows for proper handling of refunds in case of failures.Consider adding a more specific error message for the coin retrieval:
let coin = funds .first() .ok_or(ContractError::InvalidPacket { error: Some("Transfer funds must contain exactly one coin in the AMPMsg".to_string()), })? .clone();This provides a clearer error message about the expected fund structure.
tests-integration/tests/kernel_orch.rs (3)
819-1103: Consider refactoring common setup codeThe setup code in this test function is quite lengthy and similar to other tests in the file. Consider extracting the common setup logic into a helper function to improve readability and maintainability.
Overall, the test structure and logic for simulating a failed IBC transfer and verifying the refund are well-implemented.
1105-1470: Refactor common setup and remove unnecessary comments
Consider extracting the common setup logic into a helper function to improve readability and maintainability, as this setup is similar to other tests in the file.
Remove the commented-out lines at the end of the function (lines 1455-1462) as they appear to be unnecessary and may cause confusion.
Overall, the test structure and logic for simulating a successful IBC transfer followed by a failed message execution are well-implemented. The balance checks effectively verify the refund process.
Line range hint
1-1470: Excellent addition of unhappy path tests, consider overall refactoringThe new tests
test_kernel_ibc_funds_only_unhappyandtest_kernel_ibc_funds_and_execute_msg_unhappyare valuable additions to the test suite, covering important error scenarios in IBC transfers and message execution. They significantly improve the robustness of the testing for the Andromeda protocol's kernel orchestration.To further enhance the maintainability and readability of the test file, consider the following refactoring strategies:
- Extract common setup code into helper functions or fixtures.
- Create utility functions for frequently used operations (e.g., balance checking, contract instantiation).
- Use parameterized tests if possible to reduce code duplication between similar test cases.
- Consider organizing tests into logical groups using nested
modblocks if the file continues to grow.These refactorings will make the test file more manageable as it grows and easier for other developers to understand and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- contracts/os/andromeda-kernel/src/contract.rs (3 hunks)
- contracts/os/andromeda-kernel/src/execute.rs (6 hunks)
- packages/std/src/os/kernel.rs (4 hunks)
- tests-integration/tests/kernel_orch.rs (10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/os/andromeda-kernel/src/contract.rs
🧰 Additional context used
🔇 Additional comments (7)
packages/std/src/os/kernel.rs (6)
5-5: LGTM: New import for error handling.The addition of
ContractErrorimport is consistent with the new error handling mechanism introduced in this file.
48-48: LGTM: EnhancedTriggerRelayfor acknowledgment handling.The addition of
packet_ack_msgto theTriggerRelayvariant aligns with the PR objectives for handling failed acknowledgments in ICS20 transfers. This change enables the processing of acknowledgment messages, which is crucial for implementing the refund mechanism.
141-141: Consider usingAndrAddrfororiginal_sender.The addition of
original_senderto theSendMessageWithFundsvariant is consistent with the PR objectives for handling refunds. However, as mentioned in a previous review comment, consider usingAndrAddrinstead ofStringfor better type safety and consistency.
166-170: NewRefundDatastruct for refund mechanism.The
RefundDatastruct is well-aligned with the PR objectives for implementing the refund mechanism. However, consider the following suggestions from previous review comments:
- Use
AndrAddrinstead ofStringfororiginal_senderfor better type safety.- Consider using
Vec<Coin>instead ofCoinforfundsto support multiple denominations in refunds.These changes could enhance the flexibility and type safety of the refund mechanism.
172-173: Clarify the purpose ofSendMessageWithFundsResponse.The
SendMessageWithFundsResponsestruct has been added, but its purpose is not immediately clear. Could you provide more context on how this empty struct will be used? If it's a placeholder for future use, consider adding a comment to explain its intended purpose.
176-197: LGTM: NewAcknowledgementMsgenum with error handling methods.The
AcknowledgementMsgenum and itsunwrapandunwrap_errmethods provide a robust mechanism for handling acknowledgments and errors. This implementation aligns well with the PR objectives for improved error handling in ICS20 transfers. The methods are well-structured and provide clear error messages.contracts/os/andromeda-kernel/src/execute.rs (1)
11-14: LGTM: New imports for IBC acknowledgment handling.The added imports (
AcknowledgementMsg,SendMessageWithFundsResponse,IbcPacketAckMsg) are appropriate for the new acknowledgment handling functionality implemented in this file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests-integration/tests/kernel_orch.rs (2)
1125-1125: Consider reducing the wait timeThe test waits for 604,810 seconds (approximately 7 days), which seems excessive for a test scenario. Consider reducing this to a more reasonable duration unless there's a specific requirement for this timeframe.
1547-1554: Remove commented-out code blockThis commented-out code block appears to be unused and can be safely removed. If it's meant for future use, consider creating a GitHub issue to track it instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
- contracts/os/andromeda-kernel/src/execute.rs (6 hunks)
- contracts/os/andromeda-kernel/src/ibc.rs (4 hunks)
- tests-integration/tests/kernel_orch.rs (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/os/andromeda-kernel/src/ibc.rs
🧰 Additional context used
🔇 Additional comments (6)
contracts/os/andromeda-kernel/src/execute.rs (3)
41-41: LGTM: Well-implemented acknowledgment handling!The implementation correctly handles both successful and failed IBC transfer acknowledgments. On failure, it properly refunds the original sender using a bank message.
Also applies to: 64-89
738-740: LGTM: Improved error handling for missing funds.The error handling is well-implemented using idiomatic Rust with a clear error message.
752-761:⚠️ Potential issueCritical: Potential race condition in PENDING_MSG_AND_FUNDS storage.
The current implementation uses a global storage key for
PENDING_MSG_AND_FUNDS, which could lead to data being overwritten if multiple transfers occur simultaneously. This is particularly risky as it could result in incorrect refund handling.Consider using a unique key for each transfer. Here's a suggested implementation:
-PENDING_MSG_AND_FUNDS.save( +let storage_key = format!("{}:{}", channel.clone(), env.block.height); +PENDING_MSG_AND_FUNDS.save( + deps.storage, + &storage_key, &Ics20PacketInfo { sender: info.sender.into_string(), recipient: recipient.clone(), message: message.clone(), funds: coin, channel: channel.clone(), }, )?;Likely invalid or redundant comment.
tests-integration/tests/kernel_orch.rs (3)
229-229: LGTM: Method name updatesThe change from
wait_ibctoawait_packetsis consistent across all test functions and better reflects the async nature of the operation.Also applies to: 475-475, 825-825, 884-884
1181-1191: LGTM: Comprehensive balance verificationThe test properly verifies the refund logic by checking:
- Sender's balance is restored to the original amount
- Kernel contract has no remaining funds
1438-1471: LGTM: Well-structured failure scenario testThe test effectively validates the refund mechanism when a message execution fails after a successful ICS20 transfer:
- Sends funds with an invalid message that will be rejected
- Verifies initial balance deduction
- Confirms final balance is restored after refund
- Ensures kernel contract has no remaining funds
Also applies to: 1473-1482, 1535-1546
…t part of integration test
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
tests-integration/tests/kernel_orch.rs (4)
932-1216: Test verifies ICS20 transfer refund on acknowledgment error.The test correctly validates that:
- The sender's balance is restored after a failed transfer
- The kernel contract doesn't retain any funds
- The error acknowledgment triggers the refund mechanism
However, consider adding assertions to verify the intermediate states and adding comments to clarify the test flow.
Add descriptive comments before key sections:
let kernel_juno_send_request = kernel_juno + // Attempt an ICS20 transfer that will fail .execute( &ExecuteMsg::Send { message: AMPMsg {+// Simulate packet timeout by advancing time osmosis.wait_seconds(604_810).unwrap();
1218-1583: Test verifies refund when execute message fails after successful transfer.The test effectively validates that:
- The ICS20 transfer succeeds initially
- The subsequent execute message fails (UpdateLock with funds)
- The refund is triggered and completed
The test structure is clear, but there are commented-out assertions that should be cleaned up.
Remove the commented-out code block at lines 1568-1575 as it's not providing any value:
- // // For testing a successful outcome of the first packet sent out in the tx, you can use: - // if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome { - // // Packet has been successfully acknowledged and decoded, the transaction has gone through correctly - // } else { - // panic!("packet timed out"); - // // There was a decode error or the packet timed out - // // Else the packet timed-out, you may have a relayer error or something is wrong in your application - // };
1525-1528: Clarify acknowledgment message usage in error scenario.The comment at line 1525 explains that
AcknowledgementMsg::Okis used because the ICS20 transfer succeeds, but the subsequent message execution fails. This is a crucial detail for understanding the test flow. Consider enhancing the comment to make it even clearer.Enhance the comment to better explain the sequence:
- // It's Ok because the ics20 transfer is supposed to go through. We want the ExecuteMsg to fail + // Using Ok acknowledgment here because: + // 1. The ICS20 transfer itself succeeds + // 2. The failure occurs in the subsequent ExecuteMsg + // 3. This triggers the refund mechanism on the destination chain
932-1583: Consider adding test for concurrent refund scenarios.The current tests cover basic refund scenarios well, but there might be edge cases around concurrent refunds or race conditions that could be worth testing.
Consider adding tests for:
- Multiple failed transfers being refunded concurrently
- A refund being processed while a new transfer is initiated
- Race conditions between refund processing and chain state updates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- contracts/os/andromeda-kernel/src/execute.rs (6 hunks)
- tests-integration/tests/kernel_orch.rs (10 hunks)
🔇 Additional comments (3)
contracts/os/andromeda-kernel/src/execute.rs (2)
746-755:⚠️ Potential issueConsider using a unique key for pending transfers
Storing transfer information without a unique identifier could lead to race conditions if multiple transfers occur simultaneously. Consider using a composite key that includes the channel ID and sequence number.
- PENDING_MSG_AND_FUNDS.save( - deps.storage, - &Ics20PacketInfo { + let storage_key = format!("{}:{}", channel.clone(), env.block.height); + PENDING_MSG_AND_FUNDS.save( + deps.storage, + &storage_key, + &Ics20PacketInfo {Likely invalid or redundant comment.
79-82: Validate refund amount before processingThe refund logic should validate that the amount being refunded matches the original transfer amount to prevent potential fund mismatches.
tests-integration/tests/kernel_orch.rs (1)
12-12: LGTM: Import changes align with IBC acknowledgment handling requirements.The added imports for IBC-related types (
IbcAcknowledgement,IbcPacket,IbcPacketAckMsg) are necessary for implementing the failed acknowledgment handling functionality.Also applies to: 20-20, 23-23, 27-30
| resp = resp.add_submessage(SubMsg { | ||
| id: ReplyId::IBCTransfer.repr(), | ||
| msg: CosmosMsg::Ibc(msg), | ||
| gas_limit: None, | ||
| reply_on: cosmwasm_std::ReplyOn::Always, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup mechanism for successful transfers
The stored transfer information should be cleaned up after a successful transfer to prevent storage bloat. Consider adding a cleanup mechanism in the acknowledgment handler.
resp = resp.add_submessage(SubMsg {
id: ReplyId::IBCTransfer.repr(),
msg: CosmosMsg::Ibc(msg),
gas_limit: None,
- reply_on: cosmwasm_std::ReplyOn::Always,
+ reply_on: cosmwasm_std::ReplyOn::Error,
});This change ensures that we only get a reply on error, which is when we need to process refunds. For successful transfers, we don't need to maintain the stored data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resp = resp.add_submessage(SubMsg { | |
| id: ReplyId::IBCTransfer.repr(), | |
| msg: CosmosMsg::Ibc(msg), | |
| gas_limit: None, | |
| reply_on: cosmwasm_std::ReplyOn::Always, | |
| }); | |
| resp = resp.add_submessage(SubMsg { | |
| id: ReplyId::IBCTransfer.repr(), | |
| msg: CosmosMsg::Ibc(msg), | |
| gas_limit: None, | |
| reply_on: cosmwasm_std::ReplyOn::Error, | |
| }); |
| let ack: AcknowledgementMsg<SendMessageWithFundsResponse> = | ||
| from_json(packet_ack_msg.acknowledgement.data)?; |
There was a problem hiding this comment.
Add error handling for acknowledgment deserialization
The from_json call could fail if the acknowledgment data is malformed. Consider adding more context to the error message to aid debugging.
- let ack: AcknowledgementMsg<SendMessageWithFundsResponse> =
- from_json(packet_ack_msg.acknowledgement.data)?;
+ let ack: AcknowledgementMsg<SendMessageWithFundsResponse> = from_json(packet_ack_msg.acknowledgement.data)
+ .map_err(|e| ContractError::Std(StdError::generic_err(format!(
+ "Failed to parse IBC acknowledgment: {}", e
+ ))))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let ack: AcknowledgementMsg<SendMessageWithFundsResponse> = | |
| from_json(packet_ack_msg.acknowledgement.data)?; | |
| let ack: AcknowledgementMsg<SendMessageWithFundsResponse> = from_json(packet_ack_msg.acknowledgement.data) | |
| .map_err(|e| ContractError::Std(StdError::generic_err(format!( | |
| "Failed to parse IBC acknowledgment: {}", e | |
| ))))?; |
| ) -> Result<Response, ContractError> { | ||
| let refund_data = REFUND_DATA.load(deps.storage)?; | ||
| // Construct the refund message | ||
| let refund_msg = IbcMsg::Transfer { |
There was a problem hiding this comment.
We should discuss what happens if this packet fails or times out.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
Cargo.toml (1)
60-60: Consider using a more flexible version constraint for cw-orch.The strict version pinning (
=0.24.1) might cause dependency resolution issues. Consider using a more flexible constraint like^0.24.1to allow for compatible patch updates while maintaining the major version boundary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
- Cargo.toml (1 hunks)
- contracts/data-storage/andromeda-counter/Cargo.toml (1 hunks)
- contracts/finance/andromeda-splitter/Cargo.toml (1 hunks)
- contracts/fungible-tokens/andromeda-ics20/Cargo.toml (1 hunks)
- contracts/os/andromeda-adodb/Cargo.toml (1 hunks)
- contracts/os/andromeda-economics/Cargo.toml (1 hunks)
- contracts/os/andromeda-ibc-registry/Cargo.toml (1 hunks)
- contracts/os/andromeda-kernel/Cargo.toml (1 hunks)
- contracts/os/andromeda-vfs/Cargo.toml (1 hunks)
- ibc-tests/Cargo.toml (1 hunks)
- packages/andromeda-testing-e2e/Cargo.toml (1 hunks)
- tests-integration/Cargo.toml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- contracts/data-storage/andromeda-counter/Cargo.toml
- contracts/finance/andromeda-splitter/Cargo.toml
- contracts/fungible-tokens/andromeda-ics20/Cargo.toml
- contracts/os/andromeda-adodb/Cargo.toml
- contracts/os/andromeda-economics/Cargo.toml
- contracts/os/andromeda-kernel/Cargo.toml
- contracts/os/andromeda-vfs/Cargo.toml
- tests-integration/Cargo.toml
🔇 Additional comments (6)
ibc-tests/Cargo.toml (2)
14-14: LGTM! Good practice using workspace dependency.Using workspace-level dependency management for
cw-orchensures consistent versions across all packages.
14-15: Verify compatibility with IBC changes.Since this PR implements IBC acknowledgment handling, please ensure that
cw-orch-daemon = "0.24.2"is compatible with the new IBC functionality.Let's check the changelog or release notes:
✅ Verification successful
Let me gather more information about the IBC functionality in the codebase and the cw-orch-daemon dependency.
Let me check one more aspect to understand the IBC functionality requirements.
Based on the gathered information, I can now provide the final response:
cw-orch-daemonversion 0.24.2 appears safe to use with IBC functionalityThe codebase shows that IBC tests are primarily focused on validator staking functionality and don't heavily rely on advanced IBC features from cw-orch-daemon. The dependency is mainly used for basic chain interaction and staking queries. Since the workspace is already using cw-orch 0.24.1, using cw-orch-daemon 0.24.2 maintains version compatibility while providing minor improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if cw-orch-daemon 0.24.2 includes any IBC-related changes # Look for IBC-related changes in git history gh release view v0.24.2 -R cosmwasm/cw-orch 2>/dev/null || echo "Release not found" # Search for IBC-related files or changes rg -l "ibc|IBC" --type rustLength of output: 3100
Script:
#!/bin/bash # Check the actual usage of cw-orch-daemon in IBC tests rg -l "cw-orch-daemon" ibc-tests/ rg "use.*cw_orch" ibc-tests/ # Check Cargo.toml for workspace dependencies cat Cargo.toml # Look for IBC-related test setup using cw-orch rg -A 5 "fn setup|async fn setup" ibc-tests/Length of output: 2823
Script:
#!/bin/bash # Check IBC-related functionality in the tests rg -A 10 "ibc_" ibc-tests/ rg -A 10 "IbcPacket" ibc-tests/ # Check if there are any specific IBC daemon requirements rg -A 5 "Daemon.*ibc|ibc.*Daemon" ibc-tests/ # Look for any IBC acknowledgment handling rg "acknowledge|ack" ibc-tests/Length of output: 2755
contracts/os/andromeda-ibc-registry/Cargo.toml (2)
36-36: LGTM! Good practice using workspace inheritance.Moving
cw-orchto use workspace inheritance helps maintain consistency across the project and reduces maintenance overhead.
Line range hint
2-3: Verify version bump consistency across related packages.The version bump to 1.0.1 should be reflected in related packages that are affected by the IBC acknowledgment handling changes.
packages/andromeda-testing-e2e/Cargo.toml (1)
50-50: LGTM: Using workspace dependency for cw-orchThe change to use workspace-level dependency management for cw-orch aligns with the project's dependency standardization approach and matches other workspace dependencies in this file.
Cargo.toml (1)
60-60: Verify compatibility with IBC functionality.This is a major version bump of cw-orch from 0.23.0 to 0.24.1. Since this PR implements critical IBC functionality for failed acknowledgment handling, we should verify compatibility.
Let's check for any breaking changes in the dependency:
✅ Verification successful
Let me gather more information about the IBC-related usage to better understand the potential impact.
Let me check one more thing to understand how cw-orch is used in relation to IBC functionality.
Version upgrade appears safe for IBC functionality
Based on the analysis of the codebase:
- The cw-orch dependency is primarily used for test orchestration and chain interaction utilities
- No direct IBC acknowledgment handling code is found in the test files
- The cw-orch usage is focused on basic chain operations (staking, querying) rather than IBC-specific functionality
The version bump from 0.23.0 to 0.24.1 should not impact the IBC functionality as cw-orch is not directly involved in IBC message handling or acknowledgments in the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential breaking changes in cw-orch that might affect IBC functionality # Test 1: Look for IBC-related changes in cw-orch's changelog or release notes gh release view cw-orch-v0.24.1 2>/dev/null || echo "Release notes not found" # Test 2: Search for IBC-related imports from cw-orch rg -l "use.*cw_orch.*ibc" # Test 3: Check if test files import cw-orch rg -l "use.*cw_orch" "tests-integration/" "ibc-tests/"Length of output: 269
Script:
#!/bin/bash # Check the actual usage of cw-orch in IBC tests rg "use.*cw_orch" -A 5 ibc-tests/tests/validator_staking.rs ibc-tests/src/main.rs # Look for any IBC-related functionality in the codebase rg "ibc|Ibc" -A 3 ibc-tests/tests/validator_staking.rs ibc-tests/src/main.rs # Check if there are any specific version requirements or constraints cat Cargo.toml | rg "cw-orch|ibc"Length of output: 2034
Script:
#!/bin/bash # Check for IBC-related functionality in the contract interface rg -A 5 "ibc|Ibc" ibc-tests/src/contract_interface.rs # Look for any IBC acknowledgment handling code rg -A 5 "acknowledge|Acknowledge" ibc-tests/ # Check for any IBC-related test configurations rg -A 5 "setup|Setup|init" ibc-tests/src/Length of output: 1923
| cw-orch-daemon = "0.24.3" | ||
| cw-orch = "0.24.1" | ||
| cw-orch = { workspace = true } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Version misalignment detected across cw-orch and cw-orch-daemon dependencies
Found inconsistent versions across the codebase:
- Workspace defines
cw-orch = "=0.24.1" ibc-tests/Cargo.tomlusescw-orch-daemon = "0.24.2"packages/andromeda-testing-e2e/Cargo.tomlusescw-orch-daemon = "0.24.3"
This confirms potential compatibility issues. Recommend:
- Define
cw-orch-daemonin workspace dependencies - Align all instances to use workspace version
- Ensure both
cw-orchandcw-orch-daemonversions are compatible
🔗 Analysis chain
Consider aligning cw-orch-daemon version with workspace
There might be potential version compatibility issues between the fixed version of cw-orch-daemon (0.24.3) and the workspace version of cw-orch. Consider using workspace dependency for cw-orch-daemon as well to ensure version alignment.
Consider updating to use workspace dependency:
-cw-orch-daemon = "0.24.3"
+cw-orch-daemon = { workspace = true }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check workspace version of cw-orch and compare with cw-orch-daemon version
# Test: Look for workspace definition of cw-orch version
echo "Checking workspace definition for cw-orch version:"
rg -A 1 'cw-orch.*=.*' Cargo.toml
# Test: Look for other cw-orch-daemon dependencies to ensure consistency
echo -e "\nChecking other cw-orch-daemon dependencies:"
rg 'cw-orch-daemon.*=.*'
Length of output: 411
Motivation
This handles two unhappy paths:
A failed ICS20 transfer automatically refunds the sending contract but not the original sender. A refund for the original sender is triggered when the authorized wallet calls the kernel and shows that the ICS20 ack failed.
A successful ICS20 transfer followed by a failed ExecuteMsg meant to move the previously sent funds. The kernel on the destination chain attaches a Reply to the ExecuteMsg while saving Refund Data. In case the Reply is an error, a refund is triggered using
IbcMsg::TransferImplementation
The following part handles the ICS20 refund.
Second scenario:
This data is saved upon receiving a packet with the msg
SendMessageWithFunds:and this reply function handles the refund in case of a failed ExecuteMsg
Testing
test_kernel_ibc_funds_only_unhappyandtest_kernel_ibc_funds_and_execute_msg_unhappyVersion Changes
None yet
Notes
We need to check if the refund data can be overwritten prematurely.
Future work
Multihop transactions over IBC
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
cw-orchacross multiple packages to a workspace reference for greater flexibility and compatibility.