Fix NodeEntropy type mismatch in UniFFI builds #750
Conversation
Adapt the NodeEntropy initialization to account for diverging constructor signatures when the uniffi feature is active. The UniFFI layer requires shared ownership (Arc) and dynamic byte validation (Result/Vec) to facilitate memory management and error handling across the FFI boundary. This change ensures the builder receives the expected pointer type in UniFFI builds while maintaining the zero-cost stack allocation for standard Rust usage.
|
👋 Thanks for assigning @tnull as a reviewer! |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Ah, thanks, seems this slipped through.
One comment.
tests/integration_tests_rust.rs
Outdated
| builder_new.set_storage_dir_path(storage_path); | ||
| builder_new.set_chain_source_esplora(esplora_url, None); | ||
|
|
||
| let node_new = builder_new.build(node_entropy).unwrap(); |
There was a problem hiding this comment.
Ah, rather than adding extra cfg flags above, can we just follow the pattern we did elswhere and add an into() here?
| let node_new = builder_new.build(node_entropy.into()).unwrap(); |
There was a problem hiding this comment.
Added a helper function to unify entropy creation. Also used into() as suggested.
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Ah, excuse the back-and-forth, but IMO would be good to avoid the extra boilerplate here. Let's just switch the test to use a Mnemonic, which is what we did elsewhere.
| println!("\nB stopped"); | ||
| } | ||
|
|
||
| // Helper to unify entropy creation |
There was a problem hiding this comment.
Ah, actually, if need this let's rather just avoid the issue altogether by using what we do elsewhere (i.e., in TestConfig::default):
let mnemonic = generate_entropy_mnemonic(None);
let node_entropy = NodeEntropy::from_bip39_mnemonic(mnemonic, None);rather than seed bytes in fn persistence_backwards_compatibility.
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Landing this to unbreak CI, might make minor changes in a follow-up.
3 participants
Context
In 06a8108,
persistence_backwards_compatibilitywas introduced. Currently, thefrom_seed_bytesconstructor and the subsequent build method have diverging signatures depending on whether theuniffifeature is enabled.Changes
into()is used to conditionally wrapnode_entropywith an Arc or not.Why this is necessary
UniFFI requires Arc wrappers for cross-language memory management. Without this change, the integration tests fail to compile when the
uniffifeature is active because the builder receives a raw struct instead of the expected shared pointer.