Add remote-wallet crate for talking to hardware wallets #4
Conversation
Create multiple sets of cdylib and staticlib with cheaders
Misc minor cleanup, regenerate C headers
Update CI release flow to work with separate linux headers required by musl. Remove unused udev script.
Fix branch name
Fill in some basic info
These functions now receive a bytearray ptr to write the result, and return a u16 status code Remove the freeptr() method
Makes CGO compatibility much easier
Receive null-terminated CStr instead of byte arrays with len for
strings. Move most code outside unsafe {}, and give variables better
names.
Build using GNU toolchain instead Re-enable release workflow
| @@ -1,28 +1,21 @@ | |||
| /* Warning, this file is autogenerated by cbindgen. Don't modify this manually. */ | |||
There was a problem hiding this comment.
Is this header file required to be written by hand?
I think https://github.com/spacemeshos/post-rs generates the C headers when it builds the library, maybe @poszu can help here?
There was a problem hiding this comment.
I don't understand the question. It's not written by hand. There's a makefile recipe for this.
There was a problem hiding this comment.
Ah my bad. It seems like https://github.com/spacemeshos/post-rs also generates the headers using a build.rs file, but doesn't commit them to the repository. I misunderstood the files to be manually edited 😞
There was a problem hiding this comment.
Why check it in the repo though? Cbindgen can be used with build.rs to automatically generate a header when the rust project is being built. This is the approach taken by post-rs (see https://github.com/spacemeshos/post-rs/blob/755f5c2432dd7493b4c6457dce1556955fe9a94d/ffi/build.rs). It has a few advantages:
- no need for extra make targets,
- no risk of forgetting to check the header in,
- no need to manually install cbindgen (it is declared as a build dependency and fetched automatically by cargo).
The header is later bundled together with the library in release artifacts: https://github.com/spacemeshos/post-rs/blob/fbf1b886253beec659527262499d1392490ea14d/.github/workflows/ci.yml#L155-L162
There was a problem hiding this comment.
no strong opinion. personally I kind of like manually regenerating it, and being alerted when it changes.
| @@ -1,28 +1,21 @@ | |||
| /* Warning, this file is autogenerated by cbindgen. Don't modify this manually. */ | |||
There was a problem hiding this comment.
Why check it in the repo though? Cbindgen can be used with build.rs to automatically generate a header when the rust project is being built. This is the approach taken by post-rs (see https://github.com/spacemeshos/post-rs/blob/755f5c2432dd7493b4c6457dce1556955fe9a94d/ffi/build.rs). It has a few advantages:
- no need for extra make targets,
- no risk of forgetting to check the header in,
- no need to manually install cbindgen (it is declared as a build dependency and fetched automatically by cargo).
The header is later bundled together with the library in release artifacts: https://github.com/spacemeshos/post-rs/blob/fbf1b886253beec659527262499d1392490ea14d/.github/workflows/ci.yml#L155-L162
| @@ -21,5 +21,6 @@ qstring = "0.7.2" | |||
| solana-sdk = "=1.14.17" | |||
| spacemesh-derivation-path = { path = "derivation-path", version = "=0.0.1" } | |||
| spacemesh-remote-wallet = { path = "remote-wallet", version = "=0.0.1" } | |||
| spacemesh-sdkutils = { path = "sdkutils", version = "=0.0.1" } | |||
| thiserror = "1.0.40" | |||
| uriparse = "0.6.4" | |||
There was a problem hiding this comment.
To fix the rpath issue on macOS this file I think needs the following configuration:
[profile.release-clib]
inherits = "release"
strip = true
lto = true
rpath = trueThere was a problem hiding this comment.
There was a problem hiding this comment.
I'm going to go ahead and merge without this. If it's still causing issues we can open a new PR to fix it (as you did with #5)
There was a problem hiding this comment.
I'm not sure if related but I created a new issue: #12
use https://github.com/dtolnay/rust-toolchain instead Co-authored-by: Bartosz Różański <[email protected]>
Simplify the code, rename toolchain -> target to be precise Thanks @poszu
Avoid using unwrap()s and check_err! macro extensively with a simple change. split the whole function into two. The extern "C" would be a small wrapper around a rust function. It would convert a Result<Pubkey, ...> into C error code (in case of Err) and write result (in case of Ok) by @poszu Co-authored-by: Bartosz Różański <[email protected]>
Ignore HW ledger test using feature instead Thanks @poszu
Fix an import and fix unused import warning in test
to match changed function signature
Adds new remote-wallet crate for talking to ledger devices
Refactor function signature/FFI interface for existing ed25519-bip32 function to receive pointer to byte array
Remove unused/unneeded ledger-udev script (holdover forked solana code)
Move macros to shared crate, for now (we may want to clean this up in future when we improve error handling in #2)
Change FFI header files from C++ to C to more easily use them in CGO
Removes musl toolchain (it causes downstream issues with libs like libudev that aren't musl-friendly, and static linking seems to work fine with glibc)