test(hardware-wallet): exercise hardware-wallet builds and mocks in CI - #759
Merged
Nanle-code merged 7 commits intoAug 31, 2026
Merged
Conversation
|
@TheWeirdDee Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Contributor
Author
|
Pushed a follow-up commit that fixes the build-breaking bugs this PR's CI job surfaced (unrelated to hardware wallets, but they block the whole crate from compiling):
Confirmed locally: |
This was referenced Aug 26, 2026
Merged
Collaborator
|
@TheWeirdDee Please fix all CI check failure |
TheWeirdDee
added a commit
to TheWeirdDee/StarForge
that referenced
this pull request
Aug 26, 2026
…all) Same fix as Nanle-code#759/Nanle-code#830: database.rs used thiserror::Error/#[error(...)] without thiserror being a declared dependency, and passed &mut Transaction where the Migration trait expects &mut Connection (Transaction has no DerefMut). Switched the trait to &Connection. commands/mod.rs and utils/mod.rs were both missing `pub mod ai_doc_qa;` from the ai-documentation-Q&A merge (Nanle-code#718), so main.rs referenced modules that didn't exist. Without this, no CI job on this PR can even attempt to compile the crate.
…/rejection/disconnect/unsupported envelopes The hardware-wallet Cargo feature (hidapi + trezor-client) was never built or tested in CI, so its code paths silently bit-rotted. Add a dedicated CI job that builds and tests it, extract and unit-test the Ledger APDU status-word classification (approval / rejection / unsupported envelope), reorder Trezor sign_transaction to validate its input and report "not supported" before touching the device (so that path is deterministically testable without hardware), and add feature-gated tests exercising the real hidapi/trezor-client backends' disconnect (no-device) behavior. Document compatibility, security, and system-dependency notes for the feature.
TheWeirdDee
force-pushed
the
fix/666-hardware-wallet-ci
branch
from
August 27, 2026 09:21
01159dc to
9ebe58b
Compare
Same underlying crisis already fixed on fix/684-archive-path-traversal (commit e7abe69) - this branch was rebased from upstream/master directly and never received it. Cherry-picked here since the bugs are identical: bindings.rs duplicate read_spec_entries, audit.rs duplicate ci_passed field, compliance.rs missing PartialEq, ai.rs non-mut binding, templates.rs/template_analytics.rs/template_recommender.rs TemplateEntry test literals missing recently-added fields, and plugins/registry.rs missing the description field and resolve_plugin_description/plugin_list_entries helpers (also wired into commands/plugin.rs, fixing the always-blank Description column in `starforge plugin list`). Dropped the Cargo.lock hunk from the cherry-pick (adds a thiserror entry) since nothing on this branch actually uses thiserror - keeping it would desync Cargo.lock from Cargo.toml and break --locked builds.
Same batch of pre-existing bugs already fixed on fix/651-scoped-lints
and fix/684-archive-path-traversal (cherry-picked from there) - these
are all inherited from upstream master and equally affect this branch:
- tests/ai_test_assistant.rs: two of the three generator helpers it
exercises (generate_edge_case_descriptions, generate_security_checks,
generate_warnings) had been relocated from utils::ai_test_assistant
into commands::ai_test as private fns, breaking the integration test.
Moved them back to utils::ai_test_assistant as pub fns and pointed
commands::ai_test at them instead of duplicating the logic. Also
added a missing .unwrap() after analyze_contract_for_testing started
returning a Result.
- tests/template_recommendation.rs: make_entry() predated the
categories/featured/repository_url fields on TemplateEntry.
- cargo fmt --all: fixed formatting drift.
- deny.toml: removed a stale RUSTSEC-2026-0190 ignore that no longer
matches any crate in the dependency tree.
- fuzz/Cargo.lock: regenerated; had drifted out of sync with
fuzz/Cargo.toml.
- .github/workflows/benchmark-latency.yml: cargo bench passed three
positional filters but Criterion's harness only accepts one -
combined into a regex alternation. Also wrapped the PR-comment step
in try/catch since fork PRs get a read-only GITHUB_TOKEN.
- templates/registry.json: SecurityReview.findings is Option<String>
but the bundled seed data has always shipped numeric values, so the
CLI's offline fallback path could never actually parse its own
bundled registry. Converted to match the type; added a regression
test.
- src/utils/database.rs: initialize() used
`get_meta("schema_version").is_ok()` to decide whether a database
was fresh, but Ok(None) is still Ok - broke every fresh install.
Changed to `.is_some()`.
- src/utils/test_optimizer.rs: batch_tests_by_profile computed the
non-IO-bound tests and then discarded them, batching an always-empty
placeholder instead - CPU/memory-bound/general tests were silently
dropped from every batch. save_state() didn't create config_dir
before writing into it.
- tests/contract_property_tests.rs: two tests with incorrect premises
(a byte-range that could never produce "short" input, and an
auth-recording assertion checked before anything was recorded).
Owner
|
@TheWeirdDee fix conflicts |
…CI verification - tests/multisig_builder_ui.rs: update calls to match the current multisig_builder API (proposal_from_template now takes a network argument; render_progress_bar takes a &SignatureProgress computed via calculate_progress and returns a single formatted string, not a tuple). - tests/test_optimizer_integration.rs: TestOptimizer::history is a HashMap<String, TestHistory>, but call sites were passing the make_history() (String, TestHistory) tuple directly to insert(), which takes two separate arguments. Switched to extend([...]) which accepts (K, V) tuples directly. - src/utils/test_optimizer.rs: TestOptimizer::config_dir and ::cache were private, which the same integration test needs to construct the struct directly from outside the crate. Made them pub, consistent with the already-public history field. Each fix was verified with an isolated `cargo test --test <name> --no-run` compile check.
Collaborator
|
@TheWeirdDee Please resolve conflicts and fix all CI failing chcks!!! |
TheWeirdDee
added a commit
to TheWeirdDee/StarForge
that referenced
this pull request
Aug 31, 2026
8db592a merged an even newer upstream/master into this branch and reintroduced the exact same class of unresolved-duplicate-content bugs fixed in the previous commit on this branch (and in Nanle-code#759): every time this branch re-merges master, git auto-resolves overlapping-but-not- conflicting insertions by keeping both sides' content instead of one, with no conflict markers, so it silently breaks the build each time. Fixed, same root causes as before: - commands/plugin.rs: `source`/`description` fields specified twice in a PluginSummary struct literal, and plugin_list_entries() called twice into an immediately-shadowed, unused binding. - plugins/registry.rs: InstalledPlugin had `description` declared twice, and a stale duplicate resolve_plugin_description/plugin_list_entries pair shadowed the real implementation later in the file. - utils/ai_test_assistant.rs: generate_edge_case_descriptions, generate_security_checks, and generate_warnings were each defined twice (byte-identical bodies); kept one copy of each. - utils/templates.rs, utils/template_analytics.rs, utils/template_recommender.rs, tests/template_recommendation.rs: TemplateEntry test literals had changelog/repository/security_review/ categories/featured/repository_url/findings fields duplicated. - src/commands/audit.rs: AuditResult literal was missing `ci_passed`. - tests/bindings_tests.rs: all four test functions had both merge sides' bodies concatenated with a missing closing brace (this is what made Rustfmt fail outright before it could check anything else). - tests/multisig_builder_ui.rs: a `use` block had its import list duplicated with one item renamed between the two copies instead of merged. - .github/workflows/benchmark-latency.yml: two `catch` blocks chained onto one `try`, which is not valid JavaScript (failed Latency Budget Check specifically). New this time, a real bug rather than a Rust-source merge artifact: templates/registry.json (the bundled default template registry shipped in the binary) had "findings": null immediately followed by "findings": "<value>" in 9 of its SecurityReview objects — a duplicate JSON key from the same recurring merge pattern, but in data, not code. serde_json rejects it outright ("duplicate field `findings`"), which made default_registry_parses and load_registry_falls_back_to_bundled_default_when_remote_unreachable fail for real: any offline install or registry-fetch failure would have hit this and been unable to fall back to the bundled registry at all. Removed the stale `null` line, keeping the real value in each case. This PR's own change (extract_zip_archive's path-traversal/absolute- path/symlink rejection in utils/templates.rs, and its four tests) was untouched by any of this and continues to pass. Verified locally: cargo build, cargo check --all-features --tests, cargo fmt --all --check, and cargo clippy --all-features --locked -- -D warnings (CI's exact invocation) all pass. cargo test --lib templates:: passes 72/73 (this PR's four new tests plus extract_zip_archive_and_validate all included); the one local-only failure (test_publish_template_versioned_stores_by_version) is a Windows-only dirs::home_dir()-ignores-HOME artifact that does not reproduce on Linux CI. cargo test --test bindings_tests and --test multisig_builder_ui both pass in full (8/8, 4/4).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #666.
Objective
hardware-walletis an optional Cargo feature (hidapifor Ledger,trezor-clientfor Trezor) that was never actually built or tested in CI —build-and-testonly rancargo build/cargo testwith default features, and whileclippydid pass--all-features,cargo clippyalone doesn't run the code, only type-check it. As a result the feature's code paths could (and did) silently break without anyone noticing.Root cause found while fixing this
Compiling with
--features hardware-walletfor the first time surfaced thatTrezorTransport::sign_transactioncalled protobuf setters (set_network,set_transaction) that don't exist on the pinnedtrezor-client = 0.1.5— Trezor's Stellar protocol has no "raw envelope" field; it requires the transaction to be decomposed into structured per-operation messages, which was never implemented. This is exactly the kind of regression issue #666 asks CI to catch. (Someone had already partially patched the compile error onmastersince I started; I kept their fix and improved it further — see below.)What changed
CI (
.github/workflows/ci.yml)hardware-walletjob: installslibudev-dev(hidapi's Linux HID backend) andlibusb-1.0-0-dev(trezor-client'srusbtransport), then runscargo build --locked --features hardware-walletandcargo test --locked --features hardware-wallet -- --test-threads=1.-- --test-threads=1matches the existingbuild-and-testjob's own precedent, and I confirmed locally it matters here too: running the new hidapi + trezor-client tests in parallel intermittently crashed the test binary (Windows-side hidapi/libusb concurrency issue); serial execution was reliably clean.libusb-1.0-0-devto theclippyjob's deps, since it already lints with--all-featuresand needs the same headers to linktrezor-client.src/utils/hardware_wallet.rsLedgerTransport::exchangeinto standaloneclassify_status_word/check_apdu_statusfunctions, with unit tests for: approval (0x9000), rejection (0x6985/0x6982— user declined on-device), unsupported envelope (0x6D00/0x6E00/0x6A81— outdated app / wrong envelope), an unrecognized status code, and a truncated response.TrezorTransport::sign_transactionto validate the HD path and report "not supported" before opening a device session — previously it calledconnect()first, so in any environment without a physical Trezor (i.e. CI) the "not supported" message was unreachable; you'd always get "No Trezor device detected" instead, masking the real limitation. This also means the unsupported-envelope path can now be tested deterministically without hardware.map_signing_errorguidance for the unsupported-envelope case.connect/device_statusfor both Ledger and Trezor return a clear disconnect error rather than hanging or panicking.tests/hardware_wallet_integration.rswallet connect ledger --timeout 1s,wallet hw-status trezor, andwallet import ... --hardware ledgerall fail cleanly (non-zero exit, clear message) with no device attached.Docs
API_REFERENCE.md: new "Hardware wallets (Ledger / Trezor)" section coveringconnect/hw-status/hw-address/import --hardware, plus security notes (every signing op needs on-device approval, no headless signing path) and the current Trezor-signing limitation.BUILD_TROUBLESHOOTING.md: per-OS system dependencies for the feature, and added the feature-enabled build/test commands to the "simulates CI" checklist.CONTRIBUTING.md: new "Run Optional-Feature Tests" section pointing contributors at the feature before they touch this file.Out of scope (flagged, not fixed here)
stellar-xdr, which is a substantial, security-sensitive feature on its own.wallet sign --hardware trezor/tx send --hardware trezornow fail with a clear "not supported" error instead of a compile error or a misleading device error.master. While validating this change I foundcargo test --lockedcurrently fails to compile onmasterfor reasons that have nothing to do with hardware wallets — at the time I branched, 34 errors acrosstemplates.rs,database.rs,ai.rs,compliance.rs,template_analytics.rs, andtemplate_recommender.rs(missing struct fields,Option<Vec<_>>vsVec<_>mismatches, immutable-borrow errors, etc.). I did not touch any of those files — that's a separate, pre-existing issue and well outside this PR's scope. Practically, this meansbuild-and-test/clippy/smokemay show red on this PR through no fault of this change; I validatedhardware_wallet.rsin isolation (a standalone scratch crate with the same pinnedhidapi/trezor-client/clap/stellar-strkeyversions) to confirm this PR's own code is correct independent of that.Testing performed
cargo fmt --checkon every file touched by this PR (clean).hardware_wallet.rsagainst the exact pinned dependency versions (clap = 4.4.18,stellar-strkey = 0.0.9,hidapi = 2.6.5,trezor-client = 0.1.5): 22 tests pass with default features, 28 pass / 1 ignored with--features hardware-wallet(the ignored one requires a physical Ledger, as before).--test-threads=1resolves it.