feat: CW Union Transfer library #327
Conversation
WalkthroughA new CosmWasm smart contract library, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Processor
participant UnionTransferLibrary
participant zkGMContract
participant EVMRecipient
User->>Processor: Initiate transfer (function call)
Processor->>UnionTransferLibrary: Execute Transfer
UnionTransferLibrary->>UnionTransferLibrary: Validate config and balances
UnionTransferLibrary->>zkGMContract: Send encoded transfer instruction (via IBC)
zkGMContract->>EVMRecipient: Release/transfer assets on EVM chain
Poem
🪧 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: 5
🧹 Nitpick comments (13)
contracts/libraries/union-transfer/.cargo/config.toml (1)
1-3: Add Cargo aliases for build and schema generation.The
wasmandschemaaliases streamline the build and schema-generation workflow for the Union Transfer library. Consider documenting these aliases in the repository’s CONTRIBUTING or README to surface them for other developers.contracts/libraries/union-transfer/src/state.rs (1)
3-3: Add documentation for storage item
Consider adding a doc comment aboveCOUNTERto clarify its purpose, for example:/// Storage key for the transfer instruction salt counter. pub const COUNTER: Item<u64> = Item::new("counter");contracts/libraries/union-transfer/src/lib.rs (1)
5-6: Consider moving tests module declaration
For readability, you may want to place the#[cfg(test)] mod tests;after allpub modexports so that test code is clearly separated from production modules.docs/src/libraries/cosmwasm/union_transfer.md (1)
3-3: Fix grammar and improve clarity in documentation
Refine active voice and concise phrasing in two spots:
- Change “allows to transfer funds” to “allows you to transfer funds” (or “allows transferring funds”).
- Replace “take into consideration the amount of tokens” with “consider the number of tokens.”
Apply this diff:
- The **Valence Union Transfer** library allows to transfer funds over [Union](https://union.build/) from an **input account** ... + The **Valence Union Transfer** library allows you to transfer funds over [Union](https://union.build/) from an **input account** ... ... - therefore the `quote_amount` value should take into consideration the amount of tokens that the filling party will receive. + therefore the `quote_amount` value should consider the number of tokens that the filling party will receive.Also applies to: 43-43
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Did you mean “transferring”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...Valence Union Transfer** library allows to transfer funds over [Union](https://union.build/...(ALLOW_TO)
contracts/libraries/union-transfer/README.md (1)
3-3: Grammar tweak for a clearer opening sentenceThe phrase “allows to transfer funds” is missing an object and sounds awkward. A small tweak improves readability:
-The **Valence Union Transfer** library allows to transfer funds over [Union](https://union.build/) from an **input account** on a source chain to an **output account** on a destination chain. +The **Valence Union Transfer** library allows you to transfer funds over [Union](https://union.build/) from an **input account** on a source chain to an **output account** on a destination chain.🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Did you mean “transferring”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...Valence Union Transfer** library allows to transfer funds over [Union](https://union.build/...(ALLOW_TO)
contracts/libraries/union-transfer/src/tests.rs (3)
14-16: Constant name/value mismatch may confuse future maintainers
ONE_MILLIONis set to1_000_000_000_000, which is one trillion. Either rename or fix the value:-const ONE_MILLION: u128 = 1_000_000_000_000_u128; +// choose one of the following +const ONE_TRILLION: u128 = 1_000_000_000_000_u128; // or -const ONE_MILLION: u128 = 1_000_000_u128;
112-143:OptionUpdate::Set(None)wipes a field – double‑check intent
transfer_timeout,batch_instruction_version, andtransfer_instruction_versionare wrapped inOptionUpdate::Set(...). When the supplied config carriesNone, the field is explicitly set to None (i.e. cleared). If the intended semantics were “leave unchanged when caller passesNone”, useOptionUpdate::Keepinstead.Please verify to avoid unintentionally resetting contract state.
83-101:zkgm_contractin tests is a local CW address, not an EVM addressFor realism and for tests to surface address‑format bugs, consider providing an EVM‑style hex string (as expected on mainnet) instead of
self.input_addr:- self.input_addr.to_string(), + "0xe7c952d457121ba8f02df1b1d85b26de80a6f1ac".to_string(),This mirrors production usage and prevents accidental coupling to the test account address.
contracts/libraries/union-transfer/src/contract.rs (3)
194-213: Salt encoding does unnecessary round‑tripping and allocates twiceYou hash to
[u8; 32], convert to a hex string, parse that back intoBytes, then stringify it again.
This can be simplified and made allocation‑free:- let send_msg = CosmosMsg::Wasm(WasmMsg::Execute { - contract_addr: cfg.zkgm_contract.to_string(), - msg: to_json_binary(&union::ExecuteMsg::Send { - /* … */ - salt: Bytes::from_hex(hex::encode(salt)) - .map_err(|_| LibraryError::ExecutionError("Can't encode the salt.".to_string()))? - .to_string(), - instruction: bytes_instruction.to_string(), - })?, - funds, - }); + // keep salt in raw bytes – `Bytes` already implements `From<&[u8]>` + let send_msg = CosmosMsg::Wasm(WasmMsg::Execute { + contract_addr: cfg.zkgm_contract.to_string(), + msg: to_json_binary(&union::ExecuteMsg::Send { + /* … */ + salt: Bytes::from(salt).to_string(), // one allocation instead of three + instruction: bytes_instruction.to_string(), + })?, + funds, + });This removes two conversions (
hex::encodeandBytes::from_hex) and cuts memory usage.
132-137:Uint256 → U256conversion can panic in edge cases
U256::from_be_bytes(…)panics if the value does not fit in 256 bits. WhileUint256is guaranteed
to be 256‑bit, a future refactor could change that invariant.
The safe idiom isU256::from_big_endian(quote_amount.to_be_bytes().as_slice()), which never panics.Not blocking, but worth hardening.
108-130: Shadowedamountvariable may confuse future maintainersInside the
elsebranch you re‑declarelet amount = amount.u128();which shadows the outerUint128
namedamount. Later you still reference the outer variable.
Consider renaming:- let amount = amount.u128(); + let native_amount_u128 = amount.u128(); … - funds.push(coin(amount, denom)); + funds.push(coin(native_amount_u128, denom));This avoids accidental misuse of the wrong binding.
contracts/libraries/union-transfer/schema/valence-union-transfer.json (1)
70-87: Schema: missingdescriptionfor critical fieldsThe JSON‑schema is thorough but some essential parameters (
zkgm_contract,channel_id) lack explanatory
text while less important ones have it.
Describing these fields improves auto‑generated docs and front‑end validations.program-manager/schema/valence-program-manager.json (1)
2946-2967: Add descriptive documentation for TransferAmount
CurrentlyTransferAmountis defined without adescription. For consistency and better schema-generated docs (similar toIbcTransferAmount), consider adding adescriptionexplaining"full_amount"vs a fixed amount object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.toml(1 hunks)contracts/libraries/union-transfer/.cargo/config.toml(1 hunks)contracts/libraries/union-transfer/Cargo.toml(1 hunks)contracts/libraries/union-transfer/README.md(1 hunks)contracts/libraries/union-transfer/schema/valence-union-transfer.json(1 hunks)contracts/libraries/union-transfer/src/bin/schema.rs(1 hunks)contracts/libraries/union-transfer/src/contract.rs(1 hunks)contracts/libraries/union-transfer/src/lib.rs(1 hunks)contracts/libraries/union-transfer/src/msg.rs(1 hunks)contracts/libraries/union-transfer/src/state.rs(1 hunks)contracts/libraries/union-transfer/src/tests.rs(1 hunks)contracts/libraries/union-transfer/src/union.rs(1 hunks)docs/src/SUMMARY.md(1 hunks)docs/src/libraries/cosmwasm/union_transfer.md(1 hunks)program-manager/Cargo.toml(1 hunks)program-manager/schema/valence-program-manager.json(7 hunks)program-manager/src/library.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
contracts/libraries/union-transfer/src/union.rs (2)
contracts/libraries/union-transfer/src/tests.rs (1)
new(33-55)contracts/libraries/union-transfer/src/msg.rs (2)
new(133-167)new(338-372)
contracts/libraries/union-transfer/src/msg.rs (5)
packages/valence-macros/src/lib.rs (1)
valence_library_query(335-353)contracts/libraries/union-transfer/src/tests.rs (2)
new(33-55)update_config(112-143)program-manager/src/library.rs (1)
new(60-67)contracts/libraries/union-transfer/src/contract.rs (1)
update_config(234-241)packages/library-base/src/state.rs (2)
load_config(28-38)save_config(20-26)
🪛 LanguageTool
contracts/libraries/union-transfer/README.md
[grammar] ~3-~3: Did you mean “transferring”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...Valence Union Transfer** library allows to transfer funds over [Union](https://union.build/...
(ALLOW_TO)
docs/src/libraries/cosmwasm/union_transfer.md
[grammar] ~3-~3: Did you mean “transferring”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...Valence Union Transfer** library allows to transfer funds over [Union](https://union.build/...
(ALLOW_TO)
[style] ~43-~43: The phrase “take into consideration” may be wordy. To make your writing clearer, consider replacing it.
Context: ...erefore the quote_amount value should take into consideration the amount of tokens that the filling p...
(TAKE_INTO_CONSIDERATION)
[uncategorized] ~43-~43: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...alue should take into consideration the amount of tokens that the filling party will r...
(AMOUNTOF_TO_NUMBEROF)
🔇 Additional comments (18)
docs/src/SUMMARY.md (1)
46-46:✅ Verification successful
Add new Union Transfer documentation entry.
The new “Union Transfer” entry is correctly placed under the CosmWasm libraries section, matching the indentation and format of neighboring items. Ensure that
docs/src/libraries/cosmwasm/union_transfer.mdexists and is accessible.Run:
🏁 Script executed:
fd "union_transfer.md" docs/src/libraries/cosmwasmLength of output: 96
Union Transfer entry added — file exists and accessible
- Confirmed
docs/src/libraries/cosmwasm/union_transfer.mdis present and correctly placed.- Documentation entry matches indentation and format of neighboring items.
program-manager/Cargo.toml (1)
47-47: Integrate Valence Union Transfer into program-manager dependencies.Adding
valence-union-transferas a workspace dependency aligns with the new CosmWasm library crate. Confirm that the path and package name incontracts/libraries/union-transfer/Cargo.tomlmatch this entry.program-manager/src/library.rs (1)
100-100:✅ Verification successful
Add
ValenceUnionTransfervariant for new library.The new
ValenceUnionTransfer(valence_union_transfer::msg::LibraryConfig)entry integrates the Union Transfer library into the program manager. Ensure themanager_impl_library_configsmacro generates the necessary handlers for this variant, and that the JSON schema atprogram-manager/schema/valence-program-manager.jsonis updated accordingly.Run:
🏁 Script executed:
rg "ValenceUnionTransfer" -A3 program-manager/schema/valence-program-manager.jsonLength of output: 504
🏁 Script executed:
#!/bin/bash # Locate the invocation of the manager_impl_library_configs macro to verify it includes the new variant rg "manager_impl_library_configs" -C3 -n program-manager/srcLength of output: 818
🏁 Script executed:
#!/bin/bash # Extract the enum definition around the manager_impl_library_configs macro to verify the new variant inclusion sed -n '50,150p' program-manager/src/library.rsLength of output: 2952
All Verified:
ValenceUnionTransferIntegrated Successfully
TheLibraryConfigenum under the#[manager_impl_library_configs]macro now includes theValenceUnionTransfervariant, and the JSON schema atprogram-manager/schema/valence-program-manager.jsonhas been updated with both the create and update definitions for"ValenceUnionTransfer". No further changes required.Cargo.toml (1)
91-91:✅ Verification successful
Integrate
valence-union-transferinto workspace dependencies.Adding the new library under
[workspace.dependencies]withfeatures = ["library"]correctly wires it into the Cargo workspace. Verify that the pathcontracts/libraries/union-transfercontains a validCargo.tomlwith thelibraryfeature flag.Run:
🏁 Script executed:
fd "Cargo.toml" contracts/libraries/union-transferLength of output: 96
🏁 Script executed:
grep -n '^\[features\]' -n contracts/libraries/union-transfer/Cargo.toml -A5Length of output: 252
Workspace dependency integration verified
- Confirmed that
contracts/libraries/union-transfer/Cargo.tomldefines a[features]section withlibrary = [], satisfying the feature-flag requirement.contracts/libraries/union-transfer/src/state.rs (1)
1-3: Storage counter definition looks correct
TheCOUNTERstorage item is properly defined to track a uniqueu64counter for generating salts.contracts/libraries/union-transfer/src/lib.rs (1)
1-4: Module exports are correctly declared
The library entry point cleanly exposescontract,msg,state, andunionmodules. The public API surface aligns with the PR objectives.contracts/libraries/union-transfer/src/bin/schema.rs (1)
1-5: Schema generation binary is implemented as expected
Themainfunction correctly invokeswrite_api!with the appropriate message types, and imports are accurate. This will produce the JSON schema for instantiate, execute, and query interfaces.contracts/libraries/union-transfer/Cargo.toml (1)
23-24: Verifyserdeandschemarsderive features
To support#[derive(Serialize, Deserialize)]and JSON schema generation, ensure that the workspace dependencies forserdeandschemarsenable thederivefeature. Without it, derive macros in your types (e.g., inmsg.rsorstate.rs) may fail to compile.contracts/libraries/union-transfer/src/msg.rs (1)
169-193:channel_idandquote_amountare not sanity‑checked
do_validaterejects zeroamountandtransfer_timeout, but allows:
channel_id == 0quote_amount == 0Both likely indicate mis‑configuration. Consider adding similar guards to prevent accidental mis‑deployments.
program-manager/schema/valence-program-manager.json (9)
873-885: Introduce ValenceUnionTransfer to LibraryConfig oneOf
The new variant for"ValenceUnionTransfer"correctly references#/definitions/LibraryConfig14, aligning with the addition of the Union Transfer library. Ensure that this oneOf branch is also handled appropriately inprogram-manager/src/library.rs(the enum variant should match this name).
984-1007: Ensure LibraryConfig14 matches Rust contract schema
TheLibraryConfig14definition introduces all required fields for the Union Transfer library. Please verify that this JSON schema mirrors the RustInstantiateMsgorLibraryConfigstruct in theunion-transfercontract. In particular, confirm that:
- Fields marked optional here (
batch_instruction_version,transfer_instruction_version,transfer_timeout) correspond toOption<u8>/Option<u64>in Rust.- Naming (e.g.
zkgm_contract) matches the Rust field names exactly.
1442-1454: Add ValenceUnionTransfer to LibraryConfigUpdate oneOf
The update schema now includes a"ValenceUnionTransfer"branch pointing to#/definitions/LibraryConfigUpdate14. This looks correct—ensure that the program manager’s update logic recognizes and routes this variant.
1617-1736: Validate LibraryConfigUpdate14 against Rust update message
LibraryConfigUpdate14defines all optional update wrappers for the Union Transfer library. Confirm that this JSON matches the RustUpdateMsgor correspondingLibraryConfigUpdatestruct:
- Required keys (
batch_instruction_version,transfer_instruction_version,transfer_timeout) align with the Rust update API.- Use of
OptionUpdate_for_uint8/OptionUpdate_for_uint64matches how numeric updates are represented in code.
2385-2411: Approve OptionUpdate_for_uint64 definition
TheOptionUpdate_for_uint64union correctly models anonevssetpattern for u64 updates. This will be used for fields liketransfer_timeout.
2412-2438: Approve OptionUpdate_for_uint8 definition
OptionUpdate_for_uint8is consistent with the uint8 update pattern and will apply to instruction version fields.
2972-2975: Uint256 definition appears consistent
TheUint256type follows the established pattern of other Uint wrappers and includes a clear description. No issues found.
3190-3207: Approve UncheckedUnionCw20Config addition
UncheckedUnionCw20Configcleanly captures the CW20 variant withminterandtoken. This aligns withUncheckedUnionDenomConfig’s downstream reference.
3208-3236: Approve UncheckedUnionDenomConfig addition
TheUncheckedUnionDenomConfigunion properly supports bothnativeandcw20cases. Definitions and nesting look correct and consistent with other denom types.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contracts/libraries/union-transfer/src/union.rs (2)
76-136: Consider enhancing the real instruction test with more assertionsWhile the test effectively validates the decoding of a real instruction, it relies heavily on debug prints rather than assertions for some fields. Adding assertions for all important fields would make the test more robust against regressions.
// Extract and print important fields for verification println!("\nFungibleAssetOrder details:"); println!(" Sender: {}", fungible_asset_order.sender); +assert_eq!(fungible_asset_order.sender.to_string(), "0x62626e31657032756d6a366b6e3334673274746a616c73633572397738707437737634786a7537333472"); println!(" Receiver: {}", fungible_asset_order.receiver); +assert_eq!(fungible_asset_order.receiver.to_string(), "0xe7c952d457121ba8f02df1b1d85b26de80a6f1ac"); println!(" Base token: {}", fungible_asset_order.baseToken); +assert_eq!(fungible_asset_order.baseToken.to_string(), "0x7562626e"); // "ubbn" println!(" Base amount: {}", fungible_asset_order.baseAmount); +assert_eq!(fungible_asset_order.baseAmount, U256::from(35000000)); // Continue with other assertions...
93-98: Consider adding a test helper for instruction debuggingThe debug printing code appears in multiple places. Consider extracting it to a test helper function to reduce duplication and make future maintenance easier.
+#[cfg(test)] +fn print_instruction_details(instruction: &Instruction) { + println!("Instruction details:"); + println!(" Version: {}", instruction.version); + println!(" Opcode: {}", instruction.opcode); + println!(" Operand length: {} bytes", instruction.operand.len()); +} // Then in the test: -// Print decoded instruction details for debugging -println!("Instruction successfully decoded:"); -println!(" Version: {}", instruction.version); -println!(" Opcode: {}", instruction.opcode); -println!(" Operand length: {} bytes", instruction.operand.len()); +// Print decoded instruction details for debugging +println!("Instruction successfully decoded:"); +print_instruction_details(&instruction);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
contracts/libraries/union-transfer/src/union.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
contracts/libraries/union-transfer/src/union.rs (1)
Learnt from: keyleu
PR: timewave-computer/valence-protocol#327
File: contracts/libraries/union-transfer/src/union.rs:8-16
Timestamp: 2025-04-23T11:02:27.574Z
Learning: When using alloy_primitives::Bytes::from_hex() for processing hex strings in Cosmwasm contracts, additional validation at deserialization time is generally redundant as Bytes::from_hex() already validates hex string format.
🔇 Additional comments (4)
contracts/libraries/union-transfer/src/union.rs (4)
1-16: Clean implementation of execute message structureThe
ExecuteMsgenum is well-structured with appropriate fields for IBC channel communication. UsingStringforsaltandinstructionaligns with the intention to handle hex-prefixed byte data, and the comment explains this design choice clearly.Per the learnings from previous reviews, validation at deserialization time isn't necessary since
Bytes::from_hex()already provides validation when these strings are processed later in the code.
18-42: Well-defined Solidity ABI structures for cross-chain communicationThe use of the
sol!macro to define Solidity-compatible structures is an excellent approach for cross-chain interoperability. The defined structures (Instruction,Batch, andFungibleAssetOrder) match the Union protocol's requirements and provide a clean interface for ABI encoding/decoding.
54-74: Complete serialization test with proper assertionsThis test properly verifies that the
ExecuteMsg::Sendserializes to the expected JSON format. The assertion ensures the test will fail if the serialization format changes unexpectedly.
138-192: Comprehensive test for instruction building from configurationThis test thoroughly validates that a
FungibleAssetOrderis correctly constructed from aConfigobject. The assertions verify key fields, ensuring the configuration transformation logic works as expected.
Counterpart of the EVM Union Transfer library but for Cosmwasm.
Works in a very similar way as the EVM one, using ABI encoded bytes in Cosmwasm. The main difference is that here the instruction goes in a batch of instructions (this is how it's currently used for btc.union.build) instead of sending the instruction directly, having, in the end, a batch with only 1 instruction.
Most parts have been unit tested but the e2e test can't be completed because the protocol for CW is only available on Babylon mainnet and we don't have an account that can upload contracts there yet. In any case, if required, an e2e test can be done there once we have that.
Summary by CodeRabbit