Refactor remote wallet by ww3512687 · Pull Request #8423 · anza-xyz/agave

ww3512687 · 2025-10-10T05:45:50Z

Problem

Summary of Changes

Introduced a new errors.rs file to define RemoteWalletError for better error management.
Updated keypair.rs files to remove redundant imports and improve clarity.
Enhanced the README.md to provide comprehensive documentation on features and supported hardware wallets.
Implemented a transport layer for USB HID communication with hardware wallets.
Created a modular structure for wallet implementations and error types.

Fixes #

- Introduced a new `errors.rs` file to define `RemoteWalletError` for better error management. - Updated `keypair.rs` files to remove redundant imports and improve clarity. - Enhanced the `README.md` to provide comprehensive documentation on features and supported hardware wallets. - Implemented a transport layer for USB HID communication with hardware wallets. - Added support for Ledger wallets, including error handling and device management. - Created a modular structure for wallet implementations and error types.

…actor_remote_wallet # Conflicts: # remote-wallet/src/lib.rs # remote-wallet/src/remote_wallet.rs # remote-wallet/src/wallet/ledger/ledger.rs

joncinque

Apologies for the very late review on this, I was waiting for the Trezor support to land with #8378 before taking a look. You might want to look at those changes, since we did a bit to support more than just Ledger there too.

There's a lot of great stuff here, but I do want to keep things simple as much as possible, by avoiding unnecessary complication. Let me know what you think about my suggestions. Since I know you mostly want to land #7252, let's keep the refactors here as simple as possible to land the Keystone integration quicker.

From a quick look, the actual Keystone wallet implementation looks good over there, but you'll need to publish ur-registry and ur-parse-lib to crates.io if you want us to integrate, since solana-remote-wallet is also published to crates.io.

So to summarize:

let's keep this refactor simple. The rebase might be gnarly, so it might simpler to start from scratch, apologies for that 🙏
please publish those crates
we land some version of #7252

How does all that sound?

joncinque · 2025-12-03T22:19:17Z

+pub trait HardwareWalletError: Error {
+    fn code(&self) -> u16;
+    fn description(&self) -> String;
+}


I don't see the utility of this trait, is it supposed to be used as a trait ever? If not, let's remove it and have the different wallets return RemoteWalletError

joncinque · 2025-12-03T22:20:56Z

+pub trait WalletProbe {
+    fn is_supported_device(&self, device_info: &hidapi::DeviceInfo) -> bool;
+
+    fn open(&self, usb: &mut HidApi, devinfo: DeviceInfo) -> Result<Device, RemoteWalletError>;
+}


I'm not seeing the usefulness of this trait, since it obfuscates device discovery code. How about removing this and just adding new device discovery in the update_devices directly?

joncinque · 2025-12-03T22:24:21Z

+pub mod errors;
+pub mod ledger;
+pub mod types;


This directory setup is a bit overkill, since to use ledger you need to import solana_remote_wallet::wallet::ledger::wallet::LedgerWallet, which is more than a little repetitive 😅

Can we just put wallet implementations at the top level? If you really want, we can have move ledger_error.rs into ledger/error.rs and have the wallet in ledger/mod.rs, but I'd honestly just keep it as it is, or move LedgerError into ledger.rs.

joncinque · 2025-12-03T22:29:11Z

+pub trait Transport: Send {
+    fn write(&self, data: &[u8]) -> Result<usize, String>;
+    fn read(&self) -> Result<Vec<u8>, String>;
+}


This trait also looks unnecessary as just a wrapper over HidDevice, so let's remove it and keep the code as it was

joncinque · 2025-12-03T22:31:01Z

nit: let's call the file error.rs to be consistent with how the code was before, ie ledger_error.rs

joncinque · 2025-12-03T22:51:25Z

+// Logging messages
+const LOG_DEVICE_SINGULAR: &str = "";
+const LOG_DEVICE_PLURAL: &str = "s";


nit: Since these are only used in one place, it's more confusing to factor out these strings -- let's just use "" and "s" at the call site

joncinque · 2025-12-03T22:53:35Z

+            // Small delay to avoid excessive polling
+            std::thread::sleep(Duration::from_millis(100));


If we want to back off, we should let the sleep be configurable so that people's code isn't stopping in confusing ways. For example, remote-wallet is used in async runtimes in the CLI, so doing a thread::sleep could stop the whole async runtime.

Unless there's a particular use-case you're worried about, let's remove this

joncinque · 2025-12-03T22:56:08Z

+    ///
+    /// Returns the number of devices added (can be negative if devices were removed)
    #[cfg(feature = "hidapi")]
    pub fn update_devices(&self) -> Result<usize, RemoteWalletError> {


With the removal of the traits as suggested, I think this function will mostly revert back to its previous implementation. You can feel free to separate into multiple functions if it helps readability

joncinque · 2025-12-03T22:57:26Z

+/// # Type Parameters
+/// * `T` - The device info type (typically `hidapi::DeviceInfo`)
 #[allow(unused_variables)]
 pub trait RemoteWallet<T> {


Feel free to move this trait into another file if you prefer, maybe src/trait.rs?

joncinque · 2025-12-03T23:01:28Z

+    /// Check if this device has an error
+    pub fn has_error(&self) -> bool {
+        self.error.is_some()
+    }
+
+    /// Get the error message if any
+    pub fn error_message(&self) -> Option<String> {
+        self.error.as_ref().map(|e| e.to_string())
+    }
+
+    /// Check if this device is ready for use (no error and valid pubkey)
+    pub fn is_ready(&self) -> bool {
+        self.error.is_none() && self.pubkey != Pubkey::default()
+    }


Same with these, is there a use planned for these? If not, let's remove

ww3512687 · 2026-04-10T12:41:32Z

hi@joncinque ,Thanks for the careful review! Regarding these methods. Since there have been many updates to the remote-wallet implementation recently, I’ve been re-evaluating the entire logic. I’ll incorporate your feedback and submit a fresh PR with the latest changes soon. Thanks again for pointing that out!

ww3512687 · 2026-04-15T05:16:11Z

Hi @joncinque, I’ve opened a new PR based on the latest version. Could you please take a look when you have a chance? Thanks! #11944

ww3512687 and others added 5 commits July 31, 2025 15:00

cargo fmt

f543731

fix warning

8a17b1e

Merge remote-tracking branch 'origin/refactor_remote_wallet' into ref…

d20f4c8

…actor_remote_wallet # Conflicts: # remote-wallet/src/lib.rs # remote-wallet/src/remote_wallet.rs # remote-wallet/src/wallet/ledger/ledger.rs

mergify Bot added community need:merge-assist labels Oct 10, 2025

mergify Bot requested a review from a team October 10, 2025 05:46

cargo clippy format

94f4f70

joncinque reviewed Dec 3, 2025

View reviewed changes

		// Small delay to avoid excessive polling
		std::thread::sleep(Duration::from_millis(100));

Conversation

ww3512687 commented Oct 10, 2025

Problem

Summary of Changes

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ww3512687 commented Apr 10, 2026

Uh oh!

ww3512687 commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ww3512687 commented Apr 15, 2026 •

edited

Loading