feat: Add IBC denom wrap/unwrap #579
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the handling of AMP messages and IBC functionalities across several Rust files. Key updates include renaming the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
packages/std/src/os/mod.rs (1)
8-10: LGTM! Consider enhancing the comment for clarity.The addition of these constants for IBC transfer is appropriate and follows good practices. The naming is clear and adheres to Rust conventions.
Consider expanding the comment to provide more context:
-// IBC transfer port +// Constants for IBC (Inter-Blockchain Communication) transfer +// TRANSFER_PORT: Standard port for IBC transfers +// IBC_VERSION: Andromeda-specific IBC protocol versionThis additional information could help other developers understand the purpose and significance of these constants more easily.
packages/std/src/os/ibc_registry.rs (1)
86-90: Consider adding documentation comments to theHopstruct and its fieldsAdding documentation comments will improve code readability and help others understand the purpose of
Hop,port_id, andchannel_id.contracts/os/andromeda-kernel/src/ibc.rs (1)
297-297: Add unit tests for new functionsThe
testsmodule is currently empty. It's important to add unit tests for the new functionsget_counterparty_denomandgenerate_ibc_hook_transfer_messageto ensure their correctness and prevent future regressions.Do you want assistance in writing these unit tests or would you like me to open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- contracts/os/andromeda-kernel/src/execute.rs (2 hunks)
- contracts/os/andromeda-kernel/src/ibc.rs (4 hunks)
- contracts/os/andromeda-kernel/src/proto.rs (0 hunks)
- packages/std/src/amp/mod.rs (1 hunks)
- packages/std/src/error.rs (2 hunks)
- packages/std/src/os/aos_querier.rs (4 hunks)
- packages/std/src/os/ibc_registry.rs (3 hunks)
- packages/std/src/os/mod.rs (1 hunks)
- packages/std/src/testing/mock_querier.rs (7 hunks)
💤 Files with no reviewable changes (1)
- contracts/os/andromeda-kernel/src/proto.rs
🧰 Additional context used
🔇 Additional comments (21)
packages/std/src/os/mod.rs (1)
9-10: Verify usage and consider future maintenance.These constants are likely to be used in various parts of the codebase for IBC-related operations.
To ensure consistent usage, let's check where these constants are being used:
Consider the following for future maintenance:
- The
IBC_VERSIONconstant might need updates in the future as the protocol evolves. Ensure there's a process in place to review and update this constant when necessary.- If these constants are used in multiple places, consider creating a dedicated module for IBC-related constants to centralize their management.
✅ Verification successful
TRANSFER_PORT and IBC_VERSION are consistently used across the codebase with no issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of TRANSFER_PORT and IBC_VERSION constants echo "Checking usage of TRANSFER_PORT:" rg --type rust "TRANSFER_PORT" echo "Checking usage of IBC_VERSION:" rg --type rust "IBC_VERSION"Length of output: 1719
packages/std/src/error.rs (2)
741-745: LGTM! New method enhances error creation.The new
newmethod provides a convenient way to create aContractError::Stdfrom a string error message. This addition improves the ease of use when creating standard errors.
719-720: LGTM! Verify existing error handling code.The updated
InvalidDenomTracePathvariant provides more flexibility with an optional message. This change improves error reporting capabilities.Please ensure that existing error handling code is updated to accommodate this change. Run the following script to find potential affected areas:
✅ Verification successful
Verification Successful! All usages of
InvalidDenomTracePathhave been updated to handle the newmsgfield appropriately.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of InvalidDenomTracePath to verify error handling # Search for InvalidDenomTracePath usage rg --type rust "InvalidDenomTracePath" -C 3Length of output: 2366
packages/std/src/os/ibc_registry.rs (8)
22-24: ConstructornewforDenomInfois correctly implementedThe function initializes
DenomInfowith the providedbase_denomandpath.
92-96: Methodto_traceinHopis correctly implementedThe method correctly formats
port_idandchannel_idinto a trace string.
126-131: Functionhops_to_traceis correctly implementedThe function accurately concatenates the hop traces into a single string.
194-203: Testtest_unwrap_patheffectively validatesunwrap_pathwith a standard pathThe test correctly checks that the function parses the path into the expected hops.
205-219: Testtest_hops_to_tracecorrectly verifies the functionality ofhops_to_traceThe test ensures that the function produces the expected trace string from a given list of hops.
221-232: Testtest_unwrap_path_invalidappropriately checks error handling for invalid pathsThe test validates that
unwrap_pathreturns the correct error when given a path with an odd number of segments.
234-239: Testtest_empty_pathconfirms that an empty path returns an empty list of hopsThe test ensures the function handles empty input correctly.
241-246: Testtest_empty_hops_to_traceverifies that an empty list of hops results in an empty traceThe test confirms that
hops_to_tracehandles empty input as expected.packages/std/src/os/aos_querier.rs (4)
152-159: Well-implementedibc_registry_address_getterfunctionThe function correctly queries the kernel's storage to retrieve the IBC Registry's address using
IBC_REGISTRY_KEY. The implementation is consistent with existing patterns in the codebase and follows best practices.
258-270: Correct implementation ofdenom_trace_getterfunctionThe function properly constructs the query to the IBC Registry to obtain the
DenomInfofor a given denomination. Error handling is appropriately managed, and the use ofquery_wasm_smartaligns with the standard querying approach.
272-324:get_counterparty_denomfunction handles denomination transformations effectivelyThe function effectively calculates the counterparty denomination by handling both unwrapping and wrapping scenarios based on the denomination trace and source channel. The logic is sound, and error conditions are properly handled with meaningful error messages.
327-422: Comprehensive test coverage intest_get_counterparty_denomThe test function thoroughly covers various scenarios, including unwrapping, wrapping, multi-hop cases, and invalid channels. This ensures the reliability and correctness of the
get_counterparty_denomfunction. Good job on including edge cases in your tests.contracts/os/andromeda-kernel/src/execute.rs (1)
19-19: Import ofgenerate_ibc_hook_transfer_messageis correctThe function
generate_ibc_hook_transfer_messagehas been properly imported fromcrate::ibc, ensuring it's available for use in this file.packages/std/src/testing/mock_querier.rs (5)
4-10: Imports for IBC functionalities are correctly addedThe new imports for IBC functionalities and related query messages are correctly added and are necessary for the enhanced IBC features.
41-42: ConstantMOCK_IBC_REGISTRY_CONTRACTis correctly definedThe new constant
MOCK_IBC_REGISTRY_CONTRACTis appropriately added and follows the existing naming conventions for mock contract addresses.
72-81: Definition of IBC-related constants is consistent and appropriateThe added constants for IBC denominations, paths, and channels are well-defined and maintain consistency with the existing codebase. They are essential for simulating IBC interactions in the mock environment.
201-201: IntegratingMOCK_IBC_REGISTRY_CONTRACTintohandle_queryThe addition of
MOCK_IBC_REGISTRY_CONTRACTin thehandle_queryfunction correctly routes IBC registry queries to thehandle_ibc_registry_querymethod.
280-282: Adding support forIBC_REGISTRY_KEYinhandle_kernel_queryThe handling of
IBC_REGISTRY_KEYin thehandle_kernel_queryfunction appropriately returns theMOCK_IBC_REGISTRY_CONTRACTaddress, enabling proper retrieval of the IBC registry contract within the kernel.
| @@ -3,6 +3,7 @@ pub mod messages; | |||
| pub mod recipient; | |||
|
|
|||
| pub const ADO_DB_KEY: &str = "adodb"; | |||
| pub const IBC_REGISTRY_KEY: &str = "ibc-registry"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Replace hardcoded "ibc-registry" strings with IBC_REGISTRY_KEY constant.
- Found hardcoded "ibc-registry" strings in:
packages/andromeda-testing/src/mock.rs(2 occurrences)
Please update these instances to use the IBC_REGISTRY_KEY constant to ensure consistency and maintainability.
🔗 Analysis chain
LGTM! Verify usage of the new constant.
The addition of IBC_REGISTRY_KEY constant is appropriate and aligns with the PR objectives. The naming convention and placement are consistent with other constants in this file.
To ensure the new constant is being utilized as intended, let's verify its usage across the codebase:
This will help confirm that the new constant is being used appropriately and that there are no remaining hardcoded instances of the "ibc-registry" string.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of IBC_REGISTRY_KEY constant
# Search for direct usage of the constant
echo "Searching for direct usage of IBC_REGISTRY_KEY:"
rg --type rust "IBC_REGISTRY_KEY"
# Search for the string literal "ibc-registry" to catch any hardcoded usage
echo "\nSearching for hardcoded 'ibc-registry' strings:"
rg --type rust '"ibc-registry"'
Length of output: 1259
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| }; | ||
| let new_coin = Coin::new(funds.amount.u128(), new_denom); | ||
| let (counterparty_denom, _) = get_counterparty_denom(deps, &fund.denom, &channel)?; | ||
| let new_coin = Coin::new(fund.amount.u128(), counterparty_denom); |
There was a problem hiding this comment.
Possible incorrect token denomination in MsgTransfer
The MsgTransfer is being constructed with token: Some(fund.into()), which uses the original fund with the original denomination. However, after obtaining the counterparty denomination and creating new_coin, it seems the MsgTransfer should use new_coin to ensure the correct denomination is transferred to the counterparty.
Apply this diff to fix the issue:
- token: Some(fund.into()),
+ token: Some(new_coin.into()),Also applies to: 287-287
| } | ||
|
|
||
| #[allow(clippy::too_many_arguments)] | ||
| pub fn generate_transfer_message( | ||
| pub fn generate_ibc_hook_transfer_message( |
There was a problem hiding this comment.
💡 Codebase verification
All references to generate_transfer_message have not been updated
The following file still contains references to the old function name and should be updated to generate_ibc_hook_transfer_message:
contracts/fungible-tokens/andromeda-cw20-exchange/src/contract.rs
🔗 Analysis chain
Ensure all references to generate_transfer_message are updated
The function generate_transfer_message has been renamed to generate_ibc_hook_transfer_message. Please verify that all references to the old function name within the codebase have been updated accordingly to prevent any potential issues.
Run the following script to check for any remaining references to generate_transfer_message:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all occurrences of `generate_transfer_message` in the codebase
rg --type rust 'generate_transfer_message'
Length of output: 504
| @@ -618,7 +618,7 @@ impl MsgHandler { | |||
| outgoing_packets.push(outgoing_state); | |||
| OUTGOING_IBC_HOOKS_PACKETS.save(deps.storage, &outgoing_packets)?; | |||
|
|
|||
| let msg = generate_transfer_message( | |||
| let msg = generate_ibc_hook_transfer_message( | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reducing unnecessary cloning to improve performance
In the call to generate_ibc_hook_transfer_message, several clones are performed that might be unnecessary. Cloning can incur performance overhead, especially in tight loops or frequently called functions. Where possible, pass references instead of cloning values.
Apply this diff to reduce unnecessary cloning:
let msg = generate_ibc_hook_transfer_message(
- &deps.as_ref(),
- recipient.clone(),
- message.clone(),
- msg_funds.clone(),
- channel.clone(),
- env.contract.address.to_string(),
- channel_info.kernel_address.clone(),
+ deps.as_ref(),
+ &recipient,
+ &message,
+ msg_funds.clone(), // cloning might be necessary here
+ &channel,
+ env.contract.address.as_str(),
+ &channel_info.kernel_address,
env.block.time,
)?;Explanation:
- Pass references for
recipient,message,channel, andchannel_info.kernel_addressinstead of cloning. - Use
env.contract.address.as_str()to avoid allocating a newString. - Ensure that
generate_ibc_hook_transfer_messageaccepts references for these parameters.
Committable suggestion was skipped due to low confidence.
| /// Handles all IBC Registry queries. | ||
| /// | ||
| /// Returns `"ADOType"` for `ADOType` queries with code ID 1 and an error otherwise. | ||
| /// | ||
| /// Returns an error for `CodeId` queries with key `FAKE_ADODB_KEY` and 1 otherwise. | ||
| fn handle_ibc_registry_query(&self, msg: &Binary) -> QuerierResult { | ||
| match from_json(msg).unwrap() { | ||
| IBCRegistryQueryMsg::DenomInfo { denom } => match denom.as_str() { | ||
| MOCK_OSMO_IBC_DENOM_ON_ANDR => SystemResult::Ok(ContractResult::Ok( | ||
| to_json_binary(&DenomInfo { | ||
| path: MOCK_OSMO_IBC_PATH.to_string(), | ||
| base_denom: MOCK_OSMO_NATIVE_DENOM.to_string(), | ||
| }) | ||
| .unwrap(), | ||
| )), | ||
| _ => SystemResult::Ok(ContractResult::Err("Invalid Denom".to_string())), | ||
| }, | ||
| _ => SystemResult::Ok(ContractResult::Err("Not implemented".to_string())), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Incorrect function documentation in handle_ibc_registry_query
The documentation comments for handle_ibc_registry_query are incorrect and appear to be copied from another function. The comments mention ADOType and CodeId queries, which are not relevant to this function.
Please update the documentation to accurately describe the functionality of handle_ibc_registry_query. For example:
356- /// Handles all IBC Registry queries.
357-
358- /// Returns `"ADOType"` for `ADOType` queries with code ID 1 and an error otherwise.
359-
360- /// Returns an error for `CodeId` queries with key `FAKE_ADODB_KEY` and 1 otherwise.
+ /// Handles all IBC Registry queries.
+
+ /// Returns `DenomInfo` for valid denominations and an error for invalid denominations.📝 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.
| /// Handles all IBC Registry queries. | |
| /// | |
| /// Returns `"ADOType"` for `ADOType` queries with code ID 1 and an error otherwise. | |
| /// | |
| /// Returns an error for `CodeId` queries with key `FAKE_ADODB_KEY` and 1 otherwise. | |
| fn handle_ibc_registry_query(&self, msg: &Binary) -> QuerierResult { | |
| match from_json(msg).unwrap() { | |
| IBCRegistryQueryMsg::DenomInfo { denom } => match denom.as_str() { | |
| MOCK_OSMO_IBC_DENOM_ON_ANDR => SystemResult::Ok(ContractResult::Ok( | |
| to_json_binary(&DenomInfo { | |
| path: MOCK_OSMO_IBC_PATH.to_string(), | |
| base_denom: MOCK_OSMO_NATIVE_DENOM.to_string(), | |
| }) | |
| .unwrap(), | |
| )), | |
| _ => SystemResult::Ok(ContractResult::Err("Invalid Denom".to_string())), | |
| }, | |
| _ => SystemResult::Ok(ContractResult::Err("Not implemented".to_string())), | |
| } | |
| } | |
| /// Handles all IBC Registry queries. | |
| /// | |
| /// Returns `DenomInfo` for valid denominations and an error for invalid denominations. | |
| fn handle_ibc_registry_query(&self, msg: &Binary) -> QuerierResult { | |
| match from_json(msg).unwrap() { | |
| IBCRegistryQueryMsg::DenomInfo { denom } => match denom.as_str() { | |
| MOCK_OSMO_IBC_DENOM_ON_ANDR => SystemResult::Ok(ContractResult::Ok( | |
| to_json_binary(&DenomInfo { | |
| path: MOCK_OSMO_IBC_PATH.to_string(), | |
| base_denom: MOCK_OSMO_NATIVE_DENOM.to_string(), | |
| }) | |
| .unwrap(), | |
| )), | |
| _ => SystemResult::Ok(ContractResult::Err("Invalid Denom".to_string())), | |
| }, | |
| _ => SystemResult::Ok(ContractResult::Err("Not implemented".to_string())), | |
| } | |
| } |
| // Add IBC Channels to mocket | ||
| custom_querier.base.update_ibc( | ||
| TRANSFER_PORT, | ||
| &[IbcChannel::new( | ||
| IbcEndpoint { | ||
| port_id: TRANSFER_PORT.to_string(), | ||
| channel_id: MOCK_ANDR_TO_OSMO_IBC_CHANNEL.to_string(), | ||
| }, | ||
| IbcEndpoint { | ||
| port_id: TRANSFER_PORT.to_string(), | ||
| channel_id: MOCK_OSMO_TO_ANDR_IBC_CHANNEL.to_string(), | ||
| }, | ||
| IbcOrder::Unordered, | ||
| IBC_VERSION.to_string(), | ||
| String::from("connection-0"), | ||
| )], | ||
| ); |
There was a problem hiding this comment.
Typo in comment and verify correct setup of IBC channels
There is a typo in the comment on line 95~: "mocket" should be "mock". The code correctly sets up the IBC channels in the mock querier, which is vital for testing IBC functionalities.
Apply this diff to fix the typo:
95- // Add IBC Channels to mocket
95+ // Add IBC Channels to mock📝 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.
| // Add IBC Channels to mocket | |
| custom_querier.base.update_ibc( | |
| TRANSFER_PORT, | |
| &[IbcChannel::new( | |
| IbcEndpoint { | |
| port_id: TRANSFER_PORT.to_string(), | |
| channel_id: MOCK_ANDR_TO_OSMO_IBC_CHANNEL.to_string(), | |
| }, | |
| IbcEndpoint { | |
| port_id: TRANSFER_PORT.to_string(), | |
| channel_id: MOCK_OSMO_TO_ANDR_IBC_CHANNEL.to_string(), | |
| }, | |
| IbcOrder::Unordered, | |
| IBC_VERSION.to_string(), | |
| String::from("connection-0"), | |
| )], | |
| ); | |
| // Add IBC Channels to mock | |
| custom_querier.base.update_ibc( | |
| TRANSFER_PORT, | |
| &[IbcChannel::new( | |
| IbcEndpoint { | |
| port_id: TRANSFER_PORT.to_string(), | |
| channel_id: MOCK_ANDR_TO_OSMO_IBC_CHANNEL.to_string(), | |
| }, | |
| IbcEndpoint { | |
| port_id: TRANSFER_PORT.to_string(), | |
| channel_id: MOCK_OSMO_TO_ANDR_IBC_CHANNEL.to_string(), | |
| }, | |
| IbcOrder::Unordered, | |
| IBC_VERSION.to_string(), | |
| String::from("connection-0"), | |
| )], | |
| ); |
| @@ -3,6 +3,7 @@ pub mod messages; | |||
| pub mod recipient; | |||
|
|
|||
| pub const ADO_DB_KEY: &str = "adodb"; | |||
| pub const IBC_REGISTRY_KEY: &str = "ibc-registry"; | |||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
This should be in a test module like so:
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_get_counterparty_denom() {
// ...
}
}|
|
||
| #[cfg(test)] | ||
| #[test] | ||
| fn test_get_counterparty_denom() { |
There was a problem hiding this comment.
I think a test case based approach might be better
| @@ -139,3 +190,57 @@ fn test_validate_denom() { | |||
| let valid_denom = default_denom_info.get_ibc_denom(); | |||
| verify_denom(&valid_denom, &default_denom_info).unwrap() | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
Same as above with the test module
Motivation
Added denom wrap/unwrap function in AOSQuerier to use in amp messages.
Implementation
ibc-registryserviceTesting
Version Changes
NA
Notes
These changes will be coupled with amp message denom handling logic
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests