feat(chain): unify key safety and expand tx primitives beyond Cascade#2
feat(chain): unify key safety and expand tx primitives beyond Cascade#2mateeullahmalik wants to merge 4 commits intomainfrom
Conversation
All 4 previously flagged issues have been addressed. The latest commit (10f979d) only bumps
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
![roomote-v0[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
src/chain.rs
Outdated
| signing_key: &cosmrs::crypto::secp256k1::SigningKey, | ||
| creator: &str, | ||
| ) -> Result<(), SdkError> { | ||
| let (hrp, _) = creator.split_once('1').ok_or_else(|| { |
There was a problem hiding this comment.
split_once('1') splits at the first 1 in the address string. Per the bech32 spec the human-readable part (HRP) is everything before the last 1 separator. If the HRP ever contains the digit 1 (valid in bech32), this will extract the wrong prefix and silently produce an incorrect derived address for comparison. With the current lumera prefix this is harmless, but rsplit_once('1') would be correct for any bech32 address.
| let (hrp, _) = creator.split_once('1').ok_or_else(|| { | |
| let (hrp, _) = creator.rsplit_once('1').ok_or_else(|| { |
Fix it with Roo Code or mention @roomote and request a fix.
src/keys.rs
Outdated
| } | ||
|
|
||
| pub fn validate_chain_prefix(expected_address: &str, expected_hrp: &str) -> Result<(), SdkError> { | ||
| let (actual_hrp, _) = expected_address.split_once('1').ok_or_else(|| { |
There was a problem hiding this comment.
Same split_once('1') issue as in validate_signer_matches_creator: bech32 defines the HRP as everything before the last 1 separator, not the first. Should be rsplit_once('1') to handle HRPs that contain the digit 1.
| let (actual_hrp, _) = expected_address.split_once('1').ok_or_else(|| { | |
| let (actual_hrp, _) = expected_address.rsplit_once('1').ok_or_else(|| { |
Fix it with Roo Code or mention @roomote and request a fix.
| pub async fn request_action_tx( | ||
| &self, | ||
| signing_key: &cosmrs::crypto::secp256k1::SigningKey, | ||
| tx: RequestActionTxInput, | ||
| memo: impl Into<String>, | ||
| ) -> Result<RequestActionSubmitResult, SdkError> { | ||
| self.validate_signer_matches_creator(signing_key, &tx.creator)?; | ||
|
|
||
| let mut msg_bytes = Vec::new(); | ||
| MsgRequestActionProto { | ||
| creator: tx.creator.clone(), | ||
| action_type: tx.action_type, | ||
| metadata: tx.metadata, | ||
| price: tx.price, | ||
| expiration_time: tx.expiration_time, | ||
| file_size_kbs: tx.file_size_kbs, | ||
| app_pubkey: tx.app_pubkey, | ||
| } | ||
| .encode(&mut msg_bytes) | ||
| .map_err(|e| SdkError::Serialization(e.to_string()))?; | ||
|
|
||
| let any = Any { | ||
| type_url: "/lumera.action.v1.MsgRequestAction".to_string(), | ||
| value: msg_bytes, | ||
| }; | ||
|
|
||
| let (tx_raw, _gas) = self | ||
| .build_signed_tx_with_simulation( | ||
| signing_key, | ||
| &tx.creator, | ||
| vec![any], | ||
| memo, | ||
| 500_000, | ||
| 1.3, | ||
| ) | ||
| .await?; | ||
|
|
||
| let broadcast = self | ||
| .broadcast_signed_tx(&tx_raw, BroadcastMode::Commit) | ||
| .await?; | ||
|
|
||
| if broadcast.check_tx_code.unwrap_or_default() != 0 { | ||
| return Err(SdkError::Chain(format!( | ||
| "check_tx failed: {}", | ||
| broadcast.log | ||
| ))); | ||
| } | ||
| if broadcast.deliver_tx_code.unwrap_or_default() != 0 { | ||
| return Err(SdkError::Chain(format!( | ||
| "deliver_tx failed: {}", | ||
| broadcast.log | ||
| ))); | ||
| } | ||
|
|
||
| let action_id = extract_action_id_from_log(&broadcast.log).ok_or_else(|| { | ||
| SdkError::Chain(format!( | ||
| "unable to extract action_id from commit log: {}", | ||
| broadcast.log | ||
| )) | ||
| })?; | ||
|
|
||
| Ok(RequestActionSubmitResult { | ||
| tx_hash: broadcast.tx_hash, | ||
| action_id, | ||
| }) | ||
| } |
There was a problem hiding this comment.
request_action_tx submits the same MsgRequestAction as register_action, but unlike register_action it has no sequence-mismatch retry loop. If the on-chain sequence drifts (e.g. concurrent tx from the same account, or a stale REST cache), this will fail on the first attempt with a sequence error. The existing register_action handles this with a 3-attempt retry that re-fetches the account. Consider adding similar retry logic here, or extracting the retry loop into a shared helper that both methods can use.
Fix it with Roo Code or mention @roomote and request a fix.
| ) -> Result<(cosmrs::crypto::secp256k1::SigningKey, K256SigningKey), ApiError> { | ||
| let m = Mnemonic::parse(mnemonic).map_err(|e| ApiError { | ||
| error: format!("invalid LUMERA_MNEMONIC: {e}"), | ||
| })?; | ||
| let seed = m.to_seed(""); | ||
| let path = DerivationPath::from_str("m/44'/118'/0'/0/0").map_err(|e| ApiError { | ||
| error: format!("invalid derivation path: {e}"), | ||
| })?; | ||
| let xprv = XPrv::derive_from_path(seed, &path).map_err(|e| ApiError { | ||
| error: format!("failed deriving key from mnemonic: {e}"), | ||
| })?; | ||
| let sk_bytes = xprv.private_key().to_bytes(); | ||
| let chain_sk = | ||
| cosmrs::crypto::secp256k1::SigningKey::from_slice(&sk_bytes).map_err(|e| ApiError { | ||
| error: format!("failed creating chain signing key: {e}"), | ||
| })?; | ||
| let arb_sk = K256SigningKey::from_slice(&sk_bytes).map_err(|e| ApiError { | ||
| error: format!("failed creating arbitrary signing key: {e}"), | ||
| })?; | ||
| Ok((chain_sk, arb_sk)) | ||
| lumera_sdk_rs::keys::derive_signing_keys_from_mnemonic(mnemonic).map_err(|e| ApiError { | ||
| error: e.to_string(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
The function body now delegates to derive_signing_keys_from_mnemonic, but the file still imports bip32::{DerivationPath, XPrv}, bip39::Mnemonic, and std::str::FromStr (lines 4, 17-18) which were only used by the old inline derivation logic. These are now dead imports and will trigger compiler warnings (or errors under #[deny(unused_imports)]).
Fix it with Roo Code or mention @roomote and request a fix.
|
Addressed all review comments and CI red checks. Changes pushed:
Validation done locally:
Latest commit: |
1 participant
Summary
This PR delivers the next SDK-RS step requested after Cascade-first support:
Why
The Rust SDK was already good for Cascade orchestration, but chain interaction needed to be elevated toward sdk-go baseline patterns and safety discipline.
What changed
1) Key handling: one clear/safe path
src/keys.rswith unified identity derivation:SigningIdentity::from_mnemonic(...)cosmrs::crypto::secp256k1::SigningKey)k256::ecdsa::SigningKey)validate_address(...)validate_chain_prefix(...)derive_signing_keys_from_mnemonic(...)golden_devnet,ui_server) to consume centralized key derivation (removed duplicated derivation blocks).2) Chain interaction: beyond Cascade
Account + fee + tx confirmation
get_account_info(address)calculate_fee_amount(gas_limit)(derived from configured gas price)get_tx(tx_hash)wait_for_tx_confirmation(tx_hash, timeout_secs)Generic tx primitives
build_signed_tx(...)broadcast_signed_tx(...)send_any_msgs(...)BroadcastMode::{Async, Sync, Commit}Simulation-backed gas path
simulate_gas_for_tx(...)build_signed_tx_with_simulation(...)(fallback + gas-adjustment)Common Lumera wrapper
request_action_tx(...)implemented on top of generic tx path and simulation-backed gas.3) Richer event extraction helpers
extract_event_attribute_from_log(...)extract_event_attributes_from_tx_response(...)get_tx_event_attributes(tx_hash)wait_for_event_attribute(tx_hash, event_type, attr_key, timeout_secs)extract_action_id_from_log(...)now delegates to generic extraction helper.4) Docs
README.mdchain capabilities section.docs/chain-feasibility-gap-analysis.mdSafety / correctness checks added
Validation
cargo test --libpasses.Notes
This PR intentionally keeps diffs focused and incremental while opening a clean base for additional Lumera msg wrappers and deeper tx result helpers.