feat: enhance stargate test function
#153
Conversation
WalkthroughThis pull request introduces several changes across multiple files, focusing on the integration of a new Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (12)
crates/natives/src/helpers.rs (1)
33-33: Consider renaming the function for consistency.
The function name get_stargate_options still contains "stargate" despite the migration to a more generic CosmosCallback. Consider renaming to get_cosmos_options for consistency with the new unified approach.
tools/generate-bcs-go/src/main.rs (1)
Line range hint 1-93: Consider adding documentation about type generation.
The code generates crucial BCS serialization types for Go, but lacks documentation about which types are essential for the system to function correctly.
Add a comment block at the top of the file explaining:
use std::{env::current_dir, fs::File};
+
+// This tool generates Go type definitions for BCS serialization.
+// It traces essential types that need to be serialized across the Move-Go boundary.
+// Important:
+// - Add new types here when they need to be serialized between Move and Go
+// - Remove types that are no longer used in cross-language serialization
+// - Generated file: types/bcs.goprecompile/modules/initia_stdlib/sources/json.move (1)
90-90: Document the protocol requirements for signer field.
The conversion between _signer_ and signer in JSON suggests this is part of a specific protocol or standard. Consider adding documentation that:
- References the protocol specification
- Explains the expected format and validation rules for signer values
- Describes how this integrates with the larger stargate functionality
This would help maintainers understand the requirements and constraints of this field.
Also applies to: 99-99, 108-108
precompile/modules/minitia_stdlib/sources/json.move (1)
186-186: Consider enhancing test coverage for signer field conversion.
While the current test changes appropriately verify the marshal conversion from _signer_ to signer, consider adding explicit assertions to verify the unmarshal conversion from signer back to _signer_.
Example assertion to add:
assert!(
obj2._signer_ == string::utf8(b"signer"),
10
);Also applies to: 194-194, 250-250
crates/json/src/move_to_json.rs (1)
106-106: Consider adding test coverage for signer field mapping.
While the change is straightforward, it would be beneficial to add a test case in the test_convert_move_value_to_json_value function to verify the signer field mapping behavior.
Here's a suggested test case to add:
// Test struct with signer field
let mv = MoveValue::Struct(MoveStruct::WithTypes {
type_: StructTag {
address: CORE_CODE_ADDRESS,
module: ident_str!("test").into(),
name: ident_str!("TestStruct").into(),
type_args: vec![],
},
fields: vec![(
ident_str!("_signer_").into(),
MoveValue::Address(addr),
)],
});
let val = convert_move_value_to_json_value(&mv, 1).unwrap();
assert_eq!(
val,
json!({
"signer": addr.to_hex_literal(),
})
);precompile/modules/initia_stdlib/sources/token/collection.move (1)
392-405: Add documentation for the new function.
Please add documentation comments explaining:
- The purpose of this function
- The format of the returned class ID
- The special case handling for
@initia_stdcreator - Example usage and return values
Add this documentation above the function:
/// Converts a collection object to its corresponding class ID string.
/// For collections created by @initia_std, returns just the collection name.
/// For other collections, returns "move/" followed by the hex-encoded collection address.
///
/// # Examples
/// ```
/// // For @initia_std collection named "mycol"
/// assert!(collection_to_class_id(col) == "mycol");
///
/// // For other collections
/// assert!(collection_to_class_id(col) == "move/1234abcd...");
/// ```precompile/modules/minitia_stdlib/sources/token/collection.move (1)
392-405: Add documentation and test coverage for the new function.
The new collection_to_class_id function lacks:
- Documentation explaining:
- The purpose and usage of the function
- The format of the returned class ID
- The special case handling for
@minitia_std
- Test coverage for both cases:
- When creator is
@minitia_std - When creator is a different address
- When creator is
Add documentation above the function:
/// Converts a collection to its corresponding class ID.
/// For collections created by @minitia_std, returns the collection name.
/// For other collections, returns "move/" followed by the hex-encoded collection address.
/// This function is used for Cosmos Stargate message compatibility.Add test cases:
#[test(creator = @minitia_std)]
fun test_collection_to_class_id_minitia_std(creator: &signer) acquires Collection {
let name = string::utf8(b"collection_name");
create_collection_helper(creator, name);
let collection = object::address_to_object<Collection>(
create_collection_address(signer::address_of(creator), &name)
);
assert!(collection_to_class_id(collection) == name, 0);
}
#[test(creator = @0x123)]
fun test_collection_to_class_id_other(creator: &signer) acquires Collection {
let name = string::utf8(b"collection_name");
create_collection_helper(creator, name);
let collection = object::address_to_object<Collection>(
create_collection_address(signer::address_of(creator), &name)
);
let class_id = collection_to_class_id(collection);
assert!(string::sub_string(&class_id, 0, 5) == string::utf8(b"move/"), 0);
}crates/json/src/json_to_value.rs (1)
255-255: LGTM! Consider adding test coverage.
The addition of the _signer_ field mapping follows the existing pattern and is implemented correctly. However, to maintain the high standard of test coverage evident in the file, consider adding a test case for this new field mapping.
Example test case:
#[test]
fn test_deserialize_json_to_value_signer_field() {
let layout = MoveTypeLayout::Struct(MoveStructLayout::with_types(
StructTag {
address: AccountAddress::TWO,
module: ident_str!("test").into(),
name: ident_str!("WithSigner").into(),
type_args: vec![],
},
vec![MoveFieldLayout {
name: ident_str!("_signer_").into(),
layout: MoveTypeLayout::Address,
}],
));
let arg = b"{\"signer\": \"0x1\"}";
let result = deserialize_json_to_value(&module_id(), &layout, arg).unwrap();
assert!(result
.equals(&Value::struct_(Struct::pack(vec![Value::address(
AccountAddress::ONE,
)])))
.unwrap());
}precompile/modules/minitia_stdlib/sources/cosmos.move (1)
514-514: Avoid using magic numbers in assertions
In the test function test_stargate_vote, the assertion uses the hard-coded error code 1:
assert!(was_message_requested(&msg), 1);Using magic numbers can reduce code clarity and maintainability. Consider defining a constant or using a descriptive error code or message for better readability.
crates/e2e-move-tests/src/tests/cosmos.rs (3)
Line range hint 90-107: Eliminate duplicate coin initialization in tests
The test_initialize_coin setup is duplicated across multiple test cases. To enhance code readability and reduce redundancy, consider encapsulating this initialization into a reusable function or variable.
Create a helper function for coin initialization:
fn initialize_coin(symbol: &[u8], name: &str) -> TestInput<'static> {
(
AccountAddress::ONE,
"0x1::managed_coin::initialize",
vec![],
vec![
vec![0],
bcs::to_bytes(&name.as_bytes().to_vec()).unwrap(),
bcs::to_bytes(&symbol.to_vec()).unwrap(),
6u8.to_le_bytes().to_vec(),
bcs::to_bytes(&b"".to_vec()).unwrap(),
bcs::to_bytes(&b"".to_vec()).unwrap(),
],
ExpectedOutput::new(VMStatus::Executed, None, None, None),
)
}Use this function in your tests:
tests.push(initialize_coin(STAKING_SYMBOL, "Staking Denom"));Also applies to: 142-159, 220-237, 368-385
110-110: Define a constant for the Bech32 human-readable part (HRP)
The string literal "init" used as the HRP in Bech32 encoding is repeated multiple times. Defining it as a constant improves code clarity and reduces the risk of typos.
Declare a constant:
const BECH32_HRP: &str = "init";Use the constant in your Bech32 encoding:
- Hrp::parse_unchecked("init")
+ Hrp::parse_unchecked(BECH32_HRP)Also applies to: 166-166, 224-224, 296-296, 396-396, 445-445, 489-489, 534-534, 575-575
Line range hint 72-72: Use fixed AccountAddress values instead of AccountAddress::random()
Using AccountAddress::random() in tests can lead to non-deterministic behavior, making the tests harder to reproduce and debug. Consider using fixed addresses for consistent and repeatable test results.
Replace AccountAddress::random() with fixed addresses:
- let delegator_address = AccountAddress::random();
+ let delegator_address = AccountAddress::from_hex_literal("0xdeadbeef").unwrap();Also applies to: 76-76, 99-99, 103-103, 131-131, 135-135, 200-200, 204-204, 365-365, 369-369, 416-416, 420-420, 462-462, 466-466, 520-520, 524-524, 563-563, 567-567, 610-610, 614-614
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
- crates/e2e-move-tests/Cargo.toml (1 hunks)
- crates/e2e-move-tests/src/tests/cosmos.rs (21 hunks)
- crates/gas/src/initia_stdlib.rs (1 hunks)
- crates/json/src/json_to_value.rs (1 hunks)
- crates/json/src/move_to_json.rs (1 hunks)
- crates/natives/src/cosmos.rs (4 hunks)
- crates/natives/src/helpers.rs (3 hunks)
- crates/types/src/cosmos.rs (2 hunks)
- precompile/modules/initia_stdlib/sources/bech32.move (1 hunks)
- precompile/modules/initia_stdlib/sources/cosmos.move (8 hunks)
- precompile/modules/initia_stdlib/sources/crypto/secp256k1.move (1 hunks)
- precompile/modules/initia_stdlib/sources/json.move (6 hunks)
- precompile/modules/initia_stdlib/sources/token/collection.move (2 hunks)
- precompile/modules/initia_stdlib/tests/json_module_permission.move (5 hunks)
- precompile/modules/minitia_stdlib/sources/bech32.move (1 hunks)
- precompile/modules/minitia_stdlib/sources/cosmos.move (8 hunks)
- precompile/modules/minitia_stdlib/sources/crypto/secp256k1.move (1 hunks)
- precompile/modules/minitia_stdlib/sources/json.move (6 hunks)
- precompile/modules/minitia_stdlib/sources/token/collection.move (2 hunks)
- precompile/modules/minitia_stdlib/tests/json_module_permission.move (5 hunks)
- precompile/modules/move_stdlib/sources/hash.move (1 hunks)
- tools/generate-bcs-go/src/main.rs (1 hunks)
- types/bcs.go (5 hunks)
✅ Files skipped from review due to trivial changes (7)
- precompile/modules/initia_stdlib/sources/bech32.move
- precompile/modules/initia_stdlib/sources/crypto/secp256k1.move
- precompile/modules/initia_stdlib/tests/json_module_permission.move
- precompile/modules/minitia_stdlib/sources/bech32.move
- precompile/modules/minitia_stdlib/sources/crypto/secp256k1.move
- precompile/modules/minitia_stdlib/tests/json_module_permission.move
- precompile/modules/move_stdlib/sources/hash.move
🔇 Additional comments (30)
crates/natives/src/helpers.rs (2)
3-3: LGTM: Import change aligns with cosmos message unification.
The change from StargateCallback to CosmosCallback is consistent with the PR's objective of unifying the Cosmos message handling system.
Line range hint 74-79: Verify callers of get_stargate_options.
The callback instantiation looks correct, but we should verify that all callers have been updated to handle the new CosmosCallback type.
✅ Verification successful
Let me check the implementation of the caller to verify how it handles the CosmosCallback.
The caller correctly handles the new CosmosCallback type
The implementation in cosmos.rs properly handles the CosmosCallback type by:
- Correctly destructuring the tuple return value into
allow_failureandcallback - Using
is_some()to check for callback presence - Safely accessing callback fields via
as_ref().unwrap()after the null check - Properly using the callback fields (module_name, function_name) for gas charging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of get_stargate_options to verify they handle CosmosCallback
# Search for function calls
echo "Searching for direct function calls..."
rg -p "get_stargate_options\(" --type rust
# Search for potential test cases
echo -e "\nSearching for related test cases..."
rg -p "test.*stargate" --type rust
Length of output: 802
Script:
#!/bin/bash
# Check the implementation in cosmos.rs where get_stargate_options is called
rg -A 10 "get_stargate_options\(" crates/natives/src/cosmos.rs
Length of output: 534
tools/generate-bcs-go/src/main.rs (2)
13-13: LGTM! Import changes align with Stargate implementation.
The simplified import of only CosmosMessage aligns with the PR objective of consolidating cosmos messages into the Stargate implementation.
Line range hint 13-24: Verify the generated Go types.
The removal of several Cosmos-related types will affect the generated Go code. Let's verify that all necessary types are still being generated correctly.
✅ Verification successful
All required types are properly generated and implemented
The verification shows that:
- All imported types from the Rust file are correctly generated in Go
- Each type has proper struct definition, serialization, and deserialization methods
- The new unified
CosmosMessagetype is properly implemented - No legacy Cosmos-related types remain in the codebase
- Types are properly referenced in composite structures (e.g.,
ExecutionResultusingStakingDelta,CosmosMessage, andGasUsage)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the impact on generated Go types
# Expected: The generated code should only contain the new unified CosmosMessage type
# and should not contain references to removed types
# Check the generated Go file for removed types
echo "Checking for removed types in the generated code..."
rg -i "type (CosmosCoin|IBCFee|IBCHeight|IBCMessage|MoveMessage|StakingMessage)" "types/bcs.go" || echo "No old types found (expected)"
# Verify CosmosMessage is properly generated
echo "Verifying CosmosMessage structure..."
rg -A 5 "type CosmosMessage struct" "types/bcs.go"
Length of output: 523
Script:
#!/bin/bash
# Let's verify the completeness of the generated types by checking for all imports from the Rust file
# First, let's check if all the imported types from the Rust file are present in the Go code
echo "Checking for presence of imported types in Go..."
rg -A 2 "type (CosmosMessage|EntryFunction|GasUsage|ModuleBundle|ExecutionResult|Script|StakingDelta|TableInfo|ViewFunction|ViewOutput|InitiaVMConfig) " "types/bcs.go"
# Also check if these types are properly referenced/used
echo "Checking for usage of these types..."
rg "(CosmosMessage|EntryFunction|GasUsage|ModuleBundle|ExecutionResult|Script|StakingDelta|TableInfo|ViewFunction|ViewOutput|InitiaVMConfig)" "types/bcs.go"
Length of output: 7373
precompile/modules/initia_stdlib/sources/json.move (3)
90-90: LGTM: Documentation updates are clear and consistent.
The added documentation for the _signer_ field conversion follows the established pattern and clearly communicates the behavior.
Also applies to: 99-99, 108-108
133-133: LGTM: Struct field addition follows conventions.
The new _signer_ field follows the established naming pattern and type conventions used for special fields.
186-186: Consider adding more test cases for the signer field.
While the basic serialization/deserialization is tested, consider adding test cases for:
- Empty signer values
- Malformed signer values
- Missing signer field
- Invalid signer format
This would help ensure robust handling of the new field across different scenarios.
Also applies to: 194-194, 250-250
precompile/modules/minitia_stdlib/sources/json.move (3)
90-90: LGTM! Documentation updates are clear and consistent.
The added documentation for _signer_ field conversion follows the existing pattern and is consistently documented across marshal, marshal_to_string, and unmarshal functions.
Also applies to: 99-99, 108-108
133-133: LGTM! TestObject struct modification is appropriate.
The _signer_ field follows the established naming convention and uses the correct type.
90-90: Verify impact on existing JSON test data.
The addition of the _signer_/signer field conversion might affect existing JSON test data or fixtures in the codebase.
Also applies to: 108-108
✅ Verification successful
Let me check the actual usage context of "signer" in these files to understand if they are related to JSON test data.
No impact on existing JSON test data
The search results show that the _signer_ field only appears in test objects within the same JSON module files (json.move) where the conversion is being implemented. The other occurrences of "signer" in type_info.move are unrelated type assertions. The conversion is part of the implementation itself and the test cases already account for this behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing JSON test data that might contain "signer" field
rg -l '"signer"' --type-add 'move:*.move' --type move
Length of output: 271
Script:
#!/bin/bash
# Check the context of "signer" usage in the found files
rg '"signer"' --type-add 'move:*.move' --type move -C 5
Length of output: 3800
crates/gas/src/initia_stdlib.rs (2)
88-88: LGTM: Removal of redundant Cosmos gas parameters.
The removal of individual Cosmos message gas parameters (move_execute, move_script, delegate, etc.) aligns with the PR objective of consolidating all Cosmos messages into the Stargate implementation. This simplification makes the gas model more maintainable.
88-88: Verify the impact of increased gas costs.
The per-byte cost for Stargate operations has been increased from 7 to 18, matching other per-byte costs in the system. While this standardization makes sense, we should verify that this increase won't negatively impact existing operations.
Let's analyze the impact:
✅ Verification successful
Let's gather more specific information about the Stargate message implementations and their usage patterns.
Based on the code analysis, I can now generate a final response:
Gas cost increase for Stargate operations is justified
The increased per-byte cost (from 7 to 18) is appropriate because:
- The implementation charges gas per byte for multiple components: callback module name, callback function name, and message data
- The code includes proper gas metering with
context.charge()for each byte-based operation - The standardization to 18 matches other per-byte operations in the system
- Test cases demonstrate both simple and complex Stargate message scenarios, suggesting the operations will remain functional
The change maintains system consistency while properly accounting for resource usage across all Stargate message components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and analyze Stargate message usage to understand the impact
# of increased gas costs.
# Search for Stargate message implementations and tests
echo "Searching for Stargate message implementations:"
rg -l "stargate" --type rust
# Look for any existing gas benchmarks or tests
echo -e "\nSearching for gas-related tests:"
rg -l "gas.*test|test.*gas" --type rust
Length of output: 458
Script:
#!/bin/bash
# Look for Stargate message implementations and their byte handling
echo "Checking Stargate message implementations:"
rg -A 5 -B 5 "stargate" crates/natives/src/cosmos.rs
echo -e "\nChecking test cases:"
rg -A 10 -B 2 "stargate" crates/e2e-move-tests/src/tests/cosmos.rs
# Check if there are any existing gas benchmarks
echo -e "\nLooking for gas benchmarks:"
rg -A 5 "benchmark|#\[test\].*gas" crates/e2e-move-tests/src/tests/cosmos.rs
Length of output: 4447
crates/json/src/move_to_json.rs (2)
106-106: LGTM! Field name mapping follows existing pattern.
The addition of "_signer_" => "signer" case follows the established pattern for field name mapping, similar to existing cases like _type_ and _move_.
106-106: Verify the impact on Cosmos message handling.
Given that this PR aims to enhance Stargate test functions and replace Cosmos messages, we should verify that this field mapping change aligns with the expected Cosmos message structure.
Let's search for other files that might be affected by this change:
✅ Verification successful
Based on the search results, I can now provide a final response:
The _signer_ to signer field mapping is a well-documented and consistent pattern across the codebase
The change is part of a documented JSON serialization convention in both the Initia and Minitia standard libraries. The code shows that:
- This mapping is explicitly documented in the JSON module's comments
- It's used consistently in the Cosmos message handling, specifically in
PayFeeRequeststructures - The transformation is bidirectional:
_signer_→signerduring marshaling andsigner→_signer_during unmarshaling - The field is properly handled in the Cosmos module when creating messages, converting the signer's address using
address::to_sdk
The change aligns with the existing implementation and maintains consistency with the Cosmos message structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for other files that might be handling signer fields in Cosmos messages
# Test: Search for files containing both "signer" and "cosmos" keywords
rg -l "signer.*cosmos|cosmos.*signer"
# Test: Search for Move files that might be using the _signer_ field
fd -e move | xargs rg "_signer_"
# Test: Search for test files that might need updates for the new field mapping
fd -e rs | xargs rg "test.*cosmos.*signer|cosmos.*test.*signer"
Length of output: 11541
Script:
#!/bin/bash
# Let's examine the specific usage of _signer_ in the cosmos and json modules
rg -A 5 -B 5 "_signer_" precompile/modules/initia_stdlib/sources/cosmos.move precompile/modules/initia_stdlib/sources/json.move
# Check for any test files that might be using this field mapping
rg -A 5 -B 5 "\"signer\".*test|test.*\"signer\"" crates/e2e-move-tests/src/tests/cosmos.rs
# Look for any other potential signer field mappings in JSON serialization
rg -A 5 -B 5 "signer.*json|json.*signer" crates/json/src/
Length of output: 6418
precompile/modules/initia_stdlib/sources/token/collection.move (2)
25-25: LGTM! New imports are properly organized.
The added imports for binary serialization (bcs) and hex encoding are necessary for the new collection_to_class_id function and are correctly placed with other standard library imports.
Also applies to: 29-29
392-405: 🛠️ Refactor suggestion
Consider adding length validation for the generated class ID.
The function concatenates strings without validating the final length. While unlikely, extremely long collection addresses could potentially create issues in systems consuming this class ID.
Let's verify the maximum possible length:
Consider adding a constant and validation:
+ const MAX_CLASS_ID_LENGTH: u64 = 69;
+
#[view]
public fun collection_to_class_id<T: key>(collection: Object<T>): String acquires Collection {
let col = borrow(collection);
if (col.creator == @initia_std) {
return col.name
};
let metadata_addr = object::object_address(&collection);
let denom = string::utf8(b"move/");
let addr_bytes = bcs::to_bytes(&metadata_addr);
let addr_string = hex::encode_to_string(&addr_bytes);
string::append(&mut denom, addr_string);
+ assert!(
+ string::length(&denom) <= MAX_CLASS_ID_LENGTH,
+ error::invalid_state(ECLASS_ID_TOO_LONG)
+ );
return denom
}precompile/modules/minitia_stdlib/sources/token/collection.move (1)
25-25: LGTM: New imports are correctly placed and necessary.
The additions of std::bcs and std::hex imports are well-organized and required for the new class ID generation functionality.
Also applies to: 29-29
crates/types/src/cosmos.rs (3)
1-2: Necessary imports for string manipulation and debugging
The added imports core::str and std::fmt::Debug are required for string handling and implementing the custom Debug trait for CosmosMessage.
24-25: Simplification of CosmosMessage structure
Replacing the CosmosMessage enum with a struct streamlines message handling. The addition of the callback field as Option<CosmosCallback> allows for optional callbacks, enhancing flexibility.
Also applies to: 29-30
44-47: Introduction of CosmosCallback struct
The new CosmosCallback struct encapsulates callback information, enhancing the modularity and clarity of message processing.
crates/natives/src/cosmos.rs (4)
2-7: Imports are correctly updated
The new imports for CosmosMessage and CosmosMessages are appropriate, and the additions of Struct, Value, and Vector are necessary for the updated implementations.
13-15: Helper functions and interfaces are properly imported
The additions of get_stargate_options, RawSafeNative, SafeNativeBuilder, SafeNativeContext, SafeNativeResult, and safely_pop_arg are correctly included for the new native functions.
69-74: Construction of CosmosMessage is correct
The CosmosMessage is constructed with the appropriate fields: sender, data, callback, and allow_failure. This aligns with the updated CosmosMessage structure.
117-120:
Fix syntax error in natives.extend call
In the make_all function, the natives.extend call for native_requested_messages lacks proper tuple formation. Wrap the function name and pointer in a tuple as in other extend calls.
Apply this diff to fix the syntax:
#[cfg(feature = "testing")]
natives.extend([
- "requested_messages",
- native_requested_messages as RawSafeNative,
+ ("requested_messages", native_requested_messages as RawSafeNative),
]);This correction ensures consistency and prevents syntax errors during compilation.
Likely invalid or redundant comment.
precompile/modules/initia_stdlib/sources/cosmos.move (1)
494-515: Test function test_stargate_vote is well-implemented
The test function correctly verifies that stargate_vote behaves as expected by asserting that the appropriate message was requested. This ensures the integration between stargate_vote and the messaging system is functioning correctly.
types/bcs.go (5)
461-465: Addition of CosmosCallback struct is well-structured
The new CosmosCallback struct with fields Id, ModuleAddress, ModuleName, and FunctionName is properly defined and enhances the modularity of callback handling.
468-473: Correct implementation of CosmosCallback serialization
The Serialize method for CosmosCallback correctly serializes all fields in the appropriate order, ensuring proper data representation.
487-493: Accurate deserialization of CosmosCallback
The DeserializeCosmosCallback function properly deserializes each field, maintaining the integrity of the data during the deserialization process.
Line range hint 1783-1798: Optional serialization and deserialization of CosmosCallback are correctly implemented
The functions serialize_option_CosmosCallback and deserialize_option_CosmosCallback properly handle optional CosmosCallback values, ensuring robustness when the Callback field is nil.
511-515: Ensure all usages of CosmosMessage handle the new Callback field
The addition of the Callback field to CosmosMessage requires that all instances where CosmosMessage is created or processed are updated to handle this new field appropriately.
Run the following script to find all usages of CosmosMessage and verify the handling of the Callback field:
✅ Verification successful
Let me gather more information to check if there are any other files that might be using or constructing CosmosMessage objects.
Let me try one more search to find any other potential usages or implementations:
Based on the comprehensive search results, I can now provide a final response:
The Callback field is properly handled in all usages of CosmosMessage
The codebase shows that CosmosMessage is primarily used in serialization/deserialization contexts and all implementations correctly handle the Callback field:
- The serialization method properly serializes the
Callbackfield:serialize_option_CosmosCallback(obj.Callback, serializer) - The deserialization method properly deserializes the
Callbackfield:deserialize_option_CosmosCallback(deserializer) - All test cases and implementations in Rust code properly initialize the
Callbackfield (either asNoneor with a specific value) - The struct is used as part of message passing and all message handling code is aware of the optional callback field
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `CosmosMessage` to ensure the `Callback` field is properly utilized.
# Command: Search for instances where `CosmosMessage` is used.
# Expected Result: All usages should account for the new `Callback` field.
rg -A 3 -B 1 'CosmosMessage' --type go
Length of output: 3927
Script:
#!/bin/bash
# Search for any Go files that might be constructing CosmosMessage objects
ast-grep --pattern 'CosmosMessage{$$$}'
# Also search for any test files that might be using CosmosMessage
rg -g '*_test.go' 'CosmosMessage'
Length of output: 379
Script:
#!/bin/bash
# Search for any files containing CosmosMessage with more context
rg -A 5 -B 5 'CosmosMessage' --type rust --type go
# Also search specifically for struct instantiations in Go
rg '&CosmosMessage\{' --type go
Length of output: 32440
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/types/src/cosmos.rs (1)
47-52: Consider adding documentation for the CosmosCallback struct.
While the struct is well-designed, adding documentation would help explain its purpose and the meaning of each field, especially given the rename from StargateCallback.
+/// Represents a callback that can be triggered after processing a cosmos message.
+///
+/// # Fields
+/// * `id` - Unique identifier for the callback
+/// * `module_address` - The address of the module to call
+/// * `module_name` - The name of the module containing the callback function
+/// * `function_name` - The name of the function to call
#[derive(Clone, Debug, Hash, Eq, PartialEq, Serialize, Deserialize)]
pub struct CosmosCallback {
pub id: u64,
pub module_address: AccountAddress,
pub module_name: String,
pub function_name: String,
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- crates/e2e-move-tests/src/tests/cosmos.rs (21 hunks)
- crates/types/src/cosmos.rs (2 hunks)
🔇 Additional comments (5)
crates/types/src/cosmos.rs (3)
Line range hint 1-23: LGTM! Well-structured wrapper type.
The CosmosMessages wrapper provides good encapsulation with appropriate methods for accessing the inner vector.
32-43: LGTM! Safe UTF-8 handling implemented.
The Debug implementation now safely handles invalid UTF-8 data, addressing the previous review comment.
24-30: Verify the migration of existing message types.
The consolidation of various message types into a single CosmosMessage struct with generic data field is a good architectural decision. However, we should verify that all existing message types are properly migrated.
✅ Verification successful
Let me gather more information to verify the migration thoroughly.
Based on the git history and codebase analysis, I can now provide a final response:
Migration to unified CosmosMessage structure is properly implemented
The consolidation of message types into a single CosmosMessage struct is correctly implemented with all necessary changes:
- Previous enum variants (Move, Staking, Distribution, IBC, Stargate) have been removed and their functionality is now handled through the generic
datafield - All message handling code has been updated to use the new structure, as evidenced by:
- Clean removal of old message types in commit 3ce88d0
- Consistent usage in test files (crates/e2e-move-tests/src/tests/cosmos.rs)
- Updated serialization/deserialization implementations
- Proper integration with the VM and native functions
The only remaining references to old message types are in documentation files, which is expected and doesn't affect functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all previous message types are properly handled in the new implementation
# Check for any remaining references to old message types
echo "Checking for references to old message types..."
rg -l "MoveMessage|StakingMessage|DistributionMessage|IBCMessage|StargateMessage"
# Check for any potential serialization/deserialization code that might need updates
echo "Checking for potential serialization code that needs updates..."
ast-grep --pattern 'impl (Serialize|Deserialize) for $type { $$$$ }'
Length of output: 502
Script:
#!/bin/bash
# Check the history and changes of cosmos.rs to understand the migration
echo "Checking cosmos.rs history..."
git log -p crates/types/src/cosmos.rs
# Look for any message type handling or conversion code
echo "Checking for message handling code..."
rg -A 5 "CosmosMessage"
# Check for any test files that might show migration patterns
echo "Looking for related test files..."
fd "test.*\.(rs|move)$" | xargs rg "CosmosMessage"
Length of output: 32574
crates/e2e-move-tests/src/tests/cosmos.rs (2)
109-113: Repeated Bech32 encoding pattern needs refactoring.
The Bech32 address encoding with prefix "init" is repeated across multiple test functions.
Also applies to: 166-167, 223-224, 296-297, 396-397, 445-446, 489-490, 534-535, 575-576
619-624: LGTM! Stargate test implementation looks good.
The test cases for the stargate functionality are well-implemented:
- Basic stargate message test
- Stargate with callback test
The implementation aligns with the PR objectives of enhancing the stargate test function.
Also applies to: 643-653
| &delegator_address.into_bytes(), | ||
| ) | ||
| .unwrap(); | ||
| let expected_data = format!("{{\"@type\":\"/initia.mstaking.v1.MsgDelegate\",\"amount\":[{{\"amount\":\"{amount}\",\"denom\":\"{staking_denom}\"}}],\"delegator_address\":\"{delegator_cosmos_addr}\",\"validator_address\":\"{validator_address}\"}}"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using serde for JSON formatting.
Manual JSON string formatting with format! is error-prone and harder to maintain. Using serde would provide type safety and better maintainability.
Example implementation:
use serde::Serialize;
#[derive(Serialize)]
struct DelegateMessage {
#[serde(rename = "@type")]
type_url: String,
amount: Vec<CoinAmount>,
delegator_address: String,
validator_address: String,
}
#[derive(Serialize)]
struct CoinAmount {
amount: String,
denom: String,
}
// Usage
let msg = DelegateMessage {
type_url: "/initia.mstaking.v1.MsgDelegate".to_string(),
amount: vec![CoinAmount {
amount: amount.to_string(),
denom: staking_denom.to_string(),
}],
delegator_address: delegator_cosmos_addr,
validator_address: validator_address,
};
let expected_data = serde_json::to_string(&msg).unwrap();Also applies to: 168-168, 225-225, 297-298, 397-397, 446-447, 490-490, 535-535, 576-577
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/types/src/cosmos.rs (1)
47-47: Add documentation for the CosmosCallback struct.
Consider adding documentation comments to explain:
- The purpose and usage of CosmosCallback
- The significance of each field
- Any constraints or requirements for the callback parameters
Example:
/// Represents a callback configuration for Cosmos messages.
///
/// # Fields
/// * `id` - Unique identifier for the callback
/// * `module_address` - The address of the module to call
/// * `module_name` - The name of the module containing the callback function
/// * `function_name` - The name of the function to call
crates/e2e-move-tests/src/tests/cosmos.rs (1)
4-4: Consider adding test cases for error scenarios.
The test suite thoroughly covers successful cases but could benefit from additional tests for:
- Invalid addresses
- Malformed JSON data
- Callback failures
Would you like me to help generate test cases for these error scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- crates/e2e-move-tests/src/tests/cosmos.rs (21 hunks)
- crates/types/src/cosmos.rs (2 hunks)
🔇 Additional comments (7)
crates/types/src/cosmos.rs (4)
1-2: LGTM! Appropriate imports for the implementation.
The imports are correctly structured, with core::str potentially enabling no_std compatibility.
36-39: LGTM! Proper handling of invalid UTF-8 data.
The implementation now safely handles invalid UTF-8 data using unwrap_or("<invalid UTF-8>"), addressing the previous review comment.
35-35: Verify sender address string format.
The use of to_standard_string() for the sender address suggests this might be converting to a bech32 format. Let's verify this is the intended format for debugging.
✅ Verification successful
Based on the code review, I can now provide a final response regarding the use of to_standard_string() for the sender address.
The use of to_standard_string() is appropriate for debug formatting
The to_standard_string() method is correctly used in the Debug implementation for CosmosMessage as it provides a standardized string representation of the sender address. This is consistent with how addresses are handled throughout the codebase, particularly in debug and display contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other address string conversions for consistency
rg "to_standard_string\(\)" -A 2 -B 2
Length of output: 816
Script:
#!/bin/bash
# Let's check the implementation and usage of to_standard_string
ast-grep --pattern 'fn to_standard_string($$$)'
# Also check where AccountAddress is defined and used
rg "struct AccountAddress" -A 5
Length of output: 83
Script:
#!/bin/bash
# Let's look for AccountAddress type and its methods
rg "AccountAddress" -B 2 -A 2
# Also check the file content where CosmosMessage is defined
fd -e rs cosmos.rs --exec cat {}
Length of output: 167628
24-30: Verify the impact of consolidating message types.
The consolidation of multiple message types into a single CosmosMessage struct with a generic data field is a significant architectural change. While this simplifies the message handling, we should ensure:
- All existing message types can be properly serialized into the
datafield - Type safety is maintained at the application level
- Message validation is handled appropriately
Let's verify the message type usage:
crates/e2e-move-tests/src/tests/cosmos.rs (3)
5-5: LGTM! Import changes align with the new message structure.
The simplified imports reflect the transition to a unified CosmosMessage structure, and the addition of bech32 is appropriate for the new address encoding requirements.
Also applies to: 10-10
Line range hint 621-655: LGTM! The Stargate message handling looks correct.
The test cases appropriately verify:
- Basic Stargate message sending
- Stargate message with callback functionality
128-133: Verify the new CosmosMessage structure usage.
The transition to the unified CosmosMessage structure appears correct, but we should verify that all message types are properly handled.
Also applies to: 179-184, 246-251, 318-323, 417-422, 464-469, 509-514, 552-557, 593-598, 621-626, 645-655
✅ Verification successful
CosmosMessage structure usage is consistent and correct
The verification shows that:
- The
CosmosMessageis properly defined as a struct with all required fields (sender, data, allow_failure, callback) - All usages in the codebase follow the new unified structure consistently
- No old enum variants (Staking/Distribution/IBC) are present in the codebase
- All instantiations in the test file follow the same pattern with correct field initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of CosmosMessage structure
# Test: Search for any remaining usage of old message types
rg -l "CosmosMessage::(Staking|Distribution|IBC)"
# Test: Verify all CosmosMessage instantiations follow the new structure
ast-grep --pattern 'CosmosMessage {
sender: $_,
data: $_,
allow_failure: $_,
callback: $_,
}'
Length of output: 163
Script:
#!/bin/bash
# Let's try a different approach to verify CosmosMessage usage
# Search for all CosmosMessage usages with context
rg -A 5 "CosmosMessage\s*\{"
# Search for any enum variants or old patterns
rg -A 2 "CosmosMessage::"
# Look for struct definition
rg -A 10 "struct CosmosMessage"
Length of output: 7344
| let staking_denom = str::from_utf8(STAKING_SYMBOL).unwrap(); | ||
| let delegator_cosmos_addr = bech32::encode::<Bech32>( | ||
| Hrp::parse_unchecked("init"), | ||
| &delegator_address.into_bytes(), | ||
| ) | ||
| .unwrap(); | ||
| let expected_data = format!("{{\"@type\":\"/initia.mstaking.v1.MsgDelegate\",\"amount\":[{{\"amount\":\"{amount}\",\"denom\":\"{staking_denom}\"}}],\"delegator_address\":\"{delegator_cosmos_addr}\",\"validator_address\":\"{validator_address}\"}}"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated Bech32 encoding and JSON formatting patterns.
The code shows significant duplication in two areas:
- Bech32 address encoding with the "init" prefix
- Manual JSON string formatting with
format!
Consider these improvements:
- Add a helper function for Bech32 encoding:
fn encode_init_address(address: AccountAddress) -> String {
bech32::encode::<Bech32>(
Hrp::parse_unchecked("init"),
&address.into_bytes(),
)
.unwrap()
}- Use serde for type-safe JSON formatting:
use serde::Serialize;
#[derive(Serialize)]
struct DelegateMessage {
#[serde(rename = "@type")]
type_url: String,
amount: Vec<CoinAmount>,
delegator_address: String,
validator_address: String,
}
#[derive(Serialize)]
struct CoinAmount {
amount: String,
denom: String,
}These changes would:
- Reduce code duplication
- Improve maintainability
- Provide compile-time type safety for JSON structures
- Make the code more resistant to formatting errors
Also applies to: 164-168, 222-225, 294-298, 392-397, 442-447, 489-492, 532-537, 575-578
Description
Closes: #XXXX
replace all extra cosmos message to use cosmos Stargate and introduce
requested_messagesandwas_emittedfunction.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
_signer_field in JSON serialization and deserialization.Bug Fixes
Refactor
Documentation