Repository navigation
feat: consume ts-sdk PR #554 (arkd signer rotation) + flag deprecated-signer contracts - #670
Conversation
…ed-signer contracts Build @arkade-os/sdk and @arkade-os/boltz-swap from the unpublished arkade-os/ts-sdk PR #554 (arkd signer-rotation support) and surface deprecated signers in the dev-only Contracts list. Dependency mechanism (temporary, until the SDK is published to npm): - scripts/prepare-arkade-sdk.js builds both packages from the git ref pinned in package.json ("arkadeSdk") into gitignored vendor/*.tgz. - package.json references them via file:, plus a pnpm override so boltz-swap's workspace:* sdk dependency resolves to the same build. - CI builds the tarballs from the pinned ref before install (Node 24). Feature: - Settings > Advanced > Contracts (dev mode only): each contract is classified against the operator's advertised signer set. Contracts minted under a deprecated signer show a "deprecated signer" badge, and "deprecated signer / past cutoff" once the cooperative-migration cutoff has passed. - Background auto-migration (SettlementConfig.deprecatedSignerMigration, default true) is unchanged and stays enabled.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds ChangesASP Outdated Error Handling and Contracts Screen Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Code Review — #670
Reviewer: Arkana (automated, aggressive review)
Verdict: Request changes — CI broken, merge conflict likely, minor gaps
🔴 BLOCKING: CI broken — tarball integrity mismatch
Both CI runs fail at pnpm install. The prepare-arkade-sdk.js step succeeds (builds the tarballs), but the pnpm-lock.yaml committed in this PR has hardcoded integrity hashes that don't match the tarballs produced by a fresh build:
ERR_PNPM_TARBALL_INTEGRITY Got unexpected checksum for
"vendor/arkade-os-boltz-swap.tgz".
Wanted "sha512-ocBak+y2r8vhR9rJB5X..."
Got "sha512-SvAHOVWbxGu9RBeg2P7..."
File: pnpm-lock.yaml (lines ~243, ~255 — the integrity: fields for the two file:vendor/*.tgz entries)
The lockfile was generated from one build, but CI builds fresh tarballs with different checksums (tarball contents are non-deterministic across builds/environments). Fixes:
- Preferred: Add
pnpm install --no-frozen-lockfilein CI (or--fix-lockfile) after the prepare step, OR - Don't commit the tarball integrity hashes in the lockfile — regenerate after build with
pnpm installas a CI step that precedes the "real" install
The first CI run (e2e workflow) also fails because it doesn't have the Build Arkade SDK step at all — the ci.yml changes only cover one workflow. Check if there's a second workflow file (e.g., e2e.yml or test.yml) that also needs the prepare step.
🟡 Likely merge conflict: Contracts.tsx diverged from master
The diff targets a version of src/screens/Settings/Contracts.tsx that uses Shadow, FlexRow, CopyRow components and Text tiny for badges. Current master has been refactored to use CSS class-based components (contract-card, contract-copy-field, CopyField, etc.) with a completely different ContractCard and Section signature. This PR will conflict on rebase.
Not blocking for a draft PR, but worth noting for when ts-sdk#554 merges and this is de-drafted.
🟡 Test coverage gaps
File: src/test/screens/settings/contracts.test.tsx
Three tests cover EXPIRED, MIGRATABLE, and CURRENT (active signer). Missing:
DUE_NOW— cutoff of0nmeans "migrate immediately, no cutoff advertised". The badge groups it withMIGRATABLE(orange), butDUE_NOWis semantically more urgent. Consider either a distinct badge or at minimum a test proving the current behavior is intentional.UNKNOWN_SIGNER— contract signer not in the operator's set at all. Currently renders no badge (falls through toreturn null). A test documenting this is intentional would be valuable.
🟢 Code quality — looks solid
scripts/prepare-arkade-sdk.js: Well-structured. Pinned commit hash (d748a5c), caching via ref marker, cross-drive safe (copyFileSync not rename), force-rebuild escape hatch, reuse-checkout support. The --config.ignore-scripts=true for pnpm pack is a nice touch to avoid the redundant prepack rebuild. No security concerns with the clone — the commit is pinned and verifiable.
src/screens/Settings/Contracts.tsx: The contractSignerStatus helper is properly defensive — catches exceptions, guards null signerSet and missing serverPubKey. The DeprecatedSignerBadge component is clean and does exactly what it should for a dev-gated display. signerSet is memoized once at the page level and shared across cards — correct pattern, no redundant computation.
package.json: The pnpm.overrides entry for @arkade-os/sdk correctly ensures a single SDK instance (preventing boltz-swap from resolving its workspace:* dep to a different version). The arkadeSdk config field is a clean way to pin the source of truth.
⚠️ Protocol-critical flag
This PR touches signer rotation classification — a protocol-adjacent concern. While the wallet changes here are display-only (badges on a dev-gated settings screen) and don't affect signing, migration, or VTXO handling, the upstream SDK functions (signerSetFromInfo, classifyAgainstSignerSet) are used by the SDK's migrateDeprecatedSignerVtxos and SettlementConfig.deprecatedSignerMigration for actual cooperative migration.
Requesting human review per protocol-critical policy. The display logic is correct, but a human should verify the integration assumptions (especially that aspInfo.signerPubkey and aspInfo.deprecatedSigners from the REST provider match what the classification functions expect).
Summary
| Finding | Severity | File |
|---|---|---|
| CI tarball integrity mismatch | 🔴 Blocking | pnpm-lock.yaml, .github/workflows/ci.yml |
| Missing prepare step in e2e workflow | 🔴 Blocking | .github/workflows/ (second workflow) |
| Contracts.tsx diverged from master | 🟡 Rebase needed | src/screens/Settings/Contracts.tsx |
| Missing DUE_NOW + UNKNOWN_SIGNER tests | 🟡 Coverage gap | src/test/screens/settings/contracts.test.tsx |
| Protocol-critical: needs human sign-off | — |
Good draft. Fix CI, rebase when ready, add the missing test cases.
🤖 Reviewed by Arkana
…lockfile The first CI run failed at install: pnpm defaults to a frozen lockfile under CI and rejected the rebuilt vendor/*.tgz (ERR_PNPM_TARBALL_INTEGRITY), because the tarballs are not byte-reproducible. A preinstall hook can't fix it either — pnpm resolves the file: dependencies before lifecycle scripts run, so the tarballs must exist before "pnpm install" starts. - .npmrc: frozen-lockfile=false (rebuilt tarballs have unstable integrity). - ci.yml + playwright.yml: build the SDK from the pinned ref into vendor/*.tgz in a step before "pnpm install". Note: Cloudflare Pages preview deploys are configured outside the repo and cannot run a pre-install build step, so they will fail until the SDK is published to npm.
Deploying wallet-bitcoin with
|
| Latest commit: |
8f3c9e8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5123c2d2.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://feat-signer-rotation-sdk-pr5.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
8f3c9e8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0425b199.arkade-wallet.pages.dev |
| Branch Preview URL: | https://feat-signer-rotation-sdk-pr5.arkade-wallet.pages.dev |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
8f3c9e8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://829e76be.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://feat-signer-rotation-sdk-pr5.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
Cloudflare Pages preview deploys are configured outside the repo and run a bare `pnpm install` with no pre-build step, so they could not build the unpublished SDK from a git ref. Commit the prebuilt @arkade-os/sdk + @arkade-os/boltz-swap tarballs so every install context (CI, Playwright, Cloudflare previews, local) resolves them at checkout under a frozen lockfile. - vendor/*.tgz are now tracked. Regenerate with `pnpm prepare:sdk` (builds from the ref pinned in package.json) and commit the updated tarballs plus pnpm-lock.yaml when bumping the ref. - Revert the CI/Playwright "Build Arkade SDK" steps and the Node 24 bump, and remove .npmrc: frozen-lockfile is restored because the committed tarballs have stable integrity.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commits 3e3a7f9, a03b5fad
Focus: only changes since my previous review.
🔴 Previous blockers — RESOLVED ✅
Both CI blockers from my first review are fixed:
-
Tarball integrity mismatch → Fixed by committing the prebuilt
vendor/*.tgztarballs. The lockfile integrity hashes now match the committed binaries.pnpm install --frozen-lockfileworks in all contexts. -
Missing prepare step in e2e/Playwright workflow → No longer needed. Both
ci.ymlandplaywright.ymlare unchanged from master (Node 22, no extra build steps). Clean.
Cloudflare Pages previews are now passing (were failing before). CI test checks are pending at time of review.
Approach assessment
Committing vendor tarballs is pragmatic for a draft PR. The committed binaries are from a pinned, verifiable git hash (d748a5c). The merge checklist correctly calls for removing all of this plumbing once the SDK is published.
One minor note: the vendor/*.tgz files add ~2MB of binary to git history. Not a concern for a temporary branch, but ensure the merge checklist item (revert to npm versions) is done before merging.
Remaining items from first review (unchanged, not repeating in detail)
- 🟡
Contracts.tsxwill conflict on rebase against current master (CSS refactor) - 🟡 Missing
DUE_NOWandUNKNOWN_SIGNERtest cases ⚠️ Protocol-critical (signer rotation): still needs human sign-off per policy
No new issues found in the new commits. Previous request-changes verdict stands for the test coverage gap and human sign-off requirement.
🤖 Reviewed by Arkana
Rebuild vendor/*.tgz from arkade-os/ts-sdk@8b630a0. The new commits only touch digest-mismatch error handling (ark provider) and a DigestMismatchError export; the signer-rotation classification API the wallet uses is unchanged. Also switch the contracts test's BigInt literals (1n) to BigInt(): the repo targets < ES2020, so production code uses BigInt() — CI has no tsc step, so the literals slipped through. Verified against the new build: tsc, lint, prettier, build (app + worker), 270 unit tests, and a frozen CI=true install all pass.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit e37055f9
Scope: SDK ref bump (d748a5c → 8b630a0) + BigInt literal fix in tests.
Findings
✅ Ref bump — Clean. The new ts-sdk commits (d748a5c..8b630a0) only touch digest-mismatch error handling in the ark provider and a DigestMismatchError export. The signer-rotation classification API (signerSetFromInfo, classifyAgainstSignerSet) this PR depends on is unchanged. Vendor tarball and .ref marker updated consistently.
✅ BigInt syntax fix (contracts.test.tsx:128,134,140) — Correct. The repo targets < ES2020, so BigInt(1) is the right form. 1n literals require ES2020 or later in tsconfig.json. Good catch that CI lacked a tsc step to surface this earlier.
No new issues. Previous request-changes verdict (human sign-off required for protocol-adjacent signer classification logic) still stands.
🤖 Reviewed by Arkana
Rebuild vendor/*.tgz from arkade-os/ts-sdk@00ed22d. New commits: - feat: exclude post-cutoff deprecated-signer funds from spendable balance - fix(delegate): don't send X-Build-Version to the delegate service (CORS) The signer-rotation classification API and Contract type the wallet uses are unchanged. Verified against the new build: tsc, build (app + worker), 270 unit tests, and a frozen CI=true install all pass.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit f306a861
Scope: SDK ref bump (8b630a0 → 00ed22d), vendor tarball rebuild.
SDK delta (8b630a0..00ed22d)
Two new ts-sdk commits:
fix(delegate): don't send X-Build-Version to the delegate service (CORS)— Non-critical, browser fetch header fix.feat(wallet): exclude post-cutoff deprecated-signer funds from spendable balance— Protocol-critical. AddsselectPendingRecoveryOutpoints()invtxo-manager.ts, changesgetBalance()to bucket EXPIRED-signer VTXOs underpendingRecoveryinstead ofsettled/preconfirmed, and filters them from coin selection insend(). This prevents selecting VTXOs the operator won't co-sign.
Impact on this PR
✅ None. The wallet's Contracts.tsx only uses signerSetFromInfo, classifyAgainstSignerSet, SignerSet, and SignerStatus — all unchanged. The new pendingRecoveryOutpoints(), balance bucketing, and coin-selection filtering are internal to the SDK's Wallet class and don't surface in this PR's diff.
✅ Tarball + lockfile + .ref marker all updated consistently.
Assessment
This commit is a clean ref bump. The protocol-critical logic lives entirely in ts-sdk#554 and should be reviewed there, not here. No wallet-side code changes.
Previous verdict stands: human sign-off still required (signer rotation is protocol-critical). Test coverage gaps noted in earlier reviews still apply.
🤖 Reviewed by Arkana
Rebuild vendor/*.tgz from arkade-os/ts-sdk@84267a3: - fix(providers): don't send X-Build-Version to Esplora and indexer (CORS) Provider HTTP headers only; the signer-rotation API and Contract type the wallet uses are unchanged. Verified against the new build: tsc, build (app + worker), 270 unit tests, and a frozen CI=true install all pass.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit db3e1f12
Scope: SDK ref bump (00ed22d → 84267a3), vendor tarball rebuild.
SDK delta (00ed22d..84267a3)
One ts-sdk commit:
fix(providers): don't send X-Build-Version to Esplora and indexer (CORS)— Strips a custom HTTP header from Esplora/indexer provider calls to avoid CORS preflight failures. Files changed:providers/indexer.ts,providers/onchain.ts, and related tests. Non-critical.
Impact on this PR
✅ None. Only package.json (ref field), pnpm-lock.yaml (integrity hash), vendor/.ref, and the vendor/arkade-os-sdk.tgz binary changed. The boltz-swap tarball is unchanged. The signer-rotation API (signerSetFromInfo, classifyAgainstSignerSet, SignerSet, SignerStatus) consumed by Contracts.tsx is untouched.
✅ Tarball + lockfile + .ref marker all consistent.
Assessment
Clean ref bump. No wallet-side code changes. No new issues.
Previous verdict stands: human sign-off still required (signer rotation is protocol-critical). Test coverage gaps noted in earlier reviews (DUE_NOW and UNKNOWN_SIGNER status branches) still apply.
🤖 Reviewed by Arkana
Rebuild vendor/*.tgz from arkade-os/ts-sdk@6c86a52: - fix(wallet): register baseline contracts under deprecated signers at boot - test(e2e): isolate contract-params-change from leftover deprecated signers The signer-rotation API and Contract type the wallet uses are unchanged. Verified against the new build: tsc, build (app + worker), 270 unit tests, and a frozen CI=true install all pass.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit 89909fce
Scope: SDK ref bump (84267a3 → 6c86a52), vendor tarball rebuild.
SDK delta (84267a3..6c86a52)
Two ts-sdk commits:
fix(wallet): register baseline contracts under deprecated signers at boot— Protocol-relevant. Expands the baseline contract matrix inwallet.tsto iterate over both the current server signer AND all cached deprecated signers, deduped byscriptHex. This ensures a wallet loaded with funds minted under a now-rotated signer registers those contracts at boot (not only afterrestore()). The fan-out is O(signers × timelocks × scriptTypes) which is fine — signer rotations are rare, timelocks are a small fixed set.test(e2e): isolate contract-params-change from leftover deprecated signers— Test hygiene. Adds cleanup in e2e utils to prevent deprecated signer state leaking between tests.
Impact on this PR
✅ None. Only package.json (ref field), pnpm-lock.yaml (integrity hash), vendor/.ref, and vendor/arkade-os-sdk.tgz changed. The signer-rotation API (signerSetFromInfo, classifyAgainstSignerSet, SignerSet, SignerStatus) consumed by Contracts.tsx is untouched. No wallet-side code changes.
✅ Tarball + lockfile + .ref marker all consistent.
Assessment
Clean ref bump. The protocol-critical logic (deprecated-signer contract registration at boot) lives in ts-sdk#554 and should be reviewed there.
Previous verdict stands: human sign-off still required (signer rotation is protocol-critical). Test coverage gaps noted in earlier reviews (DUE_NOW and UNKNOWN_SIGNER status branches in Contracts.tsx) still apply.
🤖 Reviewed by Arkana
Rebuild vendor/*.tgz from arkade-os/ts-sdk@41c8ee0: - fix(wallet): register boarding baseline contract under deprecated signers The signer-rotation API and Contract type the wallet uses are unchanged. Verified against the new build: tsc, build (app + worker), 270 unit tests, and a frozen CI=true install all pass.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit bdf6dcea
Scope: SDK ref bump (6c86a52 → 41c8ee0), vendor tarball rebuild.
What changed in the SDK
Single commit 41c8ee0: fix(wallet): register boarding baseline contract under deprecated signers
The ReadonlyWallet.getContractManager() now loops over baselineSigners (current + deprecated) when registering boarding contracts — previously it only registered the boarding baseline under the current signer. This means boarding UTXOs minted under a now-rotated signer are watched at boot without needing an explicit restore().
Assessment
-
Logic is correct: The boarding loop mirrors the existing default/delegate matrix fan across
baselineSigners. Dedup viaseenBaselineScriptsprevents double-registration when a boarding script collides with an already-registered default one (degenerate case whereboardingExitDelay == unilateralExitDelay). -
Test updated: The test now sets
boardingExitDelay: 288(distinct from the mock'sunilateralExitDelay: 144) so the boarding baseline is a separate contract, and asserts bothdefaultandboardingtypes exist under the deprecated signer. -
Wallet-side: Only the ref pin and vendor tarball changed — no wallet code changes.
-
⚠️ Protocol-critical: This touches signer rotation + boarding contract registration (funds at stake if a boarding UTXO under a rotated signer isn't watched). The SDK change looks correct — the loop + dedup +ensureWalletContractis the right pattern — but this needs human review on ts-sdk#554 before merging.
No new issues. Previous review findings still apply.
🤖 Reviewed by Arkana
Rework the dev-only Settings → Contracts screen to handle many contracts: - Compact, collapsible rows (type · address · deprecated-signer badge · state · age); tap to reveal address/script/parameters with copy actions. - Active/Inactive tab, type-filter chips, and a search box (matches type, address, script, params). - Virtualized list (@tanstack/react-virtual) with dynamic row heights and open-state kept in the parent so it survives rows scrolling out. - Boarding contracts now show their on-chain Bitcoin Taproot address (bc1p/tb1p/bcrt1p), derived from the P2TR script, instead of the ark encoding. - Per-section count. Tests updated for the tabbed/filtered UI (virtualizer mocked under jsdom), with added coverage for search, tab filtering, and the boarding address.
- Each contract's address links out to a block explorer: the Arkade explorer for ark addresses, the on-chain mempool explorer for boarding Bitcoin Taproot addresses (external-link icon on the expanded address row). - Much more compact filter bar: a single-line search input and one wrapping chip row (Active/Inactive · type chips · count), replacing the tall search box and segmented control. Frees vertical space so the list no longer gets cut off at the bottom (also fixed the flexbox min-height:0 bound). - Key the list/open-state by contract.script (the primary key) instead of address, which can collide across contracts (e.g. boarding) and made them expand together.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commits 68b97435, 15c910eb
Scope: Compact/searchable/filterable contracts list with virtualization, explorer links, collapsible cards.
Verdict: ✅ No blocking issues
These are purely UI changes — no protocol-critical paths touched (no VTXO handling, signing, forfeit, round, or exit logic).
Notes
-
getItemKey/openSetkeyed bycontract.script(Contracts.tsx:297,Contracts.tsx:303): The comment correctly notes thataddresscan collide across boarding contracts. Butscriptcould also collide if two contracts share the same scriptPubKey (e.g. same boarding UTXO re-registered after a sweep). If that happens, the virtualizer would assign duplicate keys (console warning, potential rendering glitch) and toggling one card would toggle both. Low probability in practice — just noting it. If contracts have a unique ID field in the SDK, that would be safer. -
typeFilternot reset on data change (Contracts.tsx:267-275): If the user selects a type filter (e.g. "vhtlc"), then the wallet context changes and the new wallet has no "vhtlc" contracts, the chip stays selected but the list shows 0 results with no visible indication of why. Minor UX papercut — consider resettingtypeFilterto'all'in theuseEffectthat fetches contracts, or filtering out stale type chips. -
onchainTaprootAddressis correct (Contracts.tsx:37-45): P2TR scriptPubKey regex, witness version 1, bech32m encoding, correct HRP mapping (bc/bcrt/tb). Falls back to the original address on any error. Display-only — no signing or spending implications. -
Test coverage is solid: Virtualizer mock is clean, new tests cover tab filtering, search, and boarding address display. The
deprecatedAspInfo/underSignerhelpers reduce boilerplate nicely. -
New dep
@tanstack/react-virtual@3.13.19: Locked in pnpm-lock, peer deps match React 18. Fine.
🤖 Reviewed by Arkana
The icon group was wrapped in a FlexRow, which defaults to width:100%, so the icons left-aligned next to the value. Add `end` (width:undefined) so the wrapper shrinks to its content and the row's space-between pushes it to the right edge, matching the other copy rows.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit 0456fba5
Scope: CSS fix — right-align copy/explorer icons in expanded contract rows.
Change
Contracts.tsx:111: Added end prop to inner <FlexRow> wrapping the copy + explorer icons in CopyRow. This pushes the icons to the right edge of the row instead of leaving them adjacent to the label text.
Assessment
✅ No issues. One-line cosmetic fix. No protocol, signing, or data-flow changes. No review action needed.
Previous review findings still apply (human sign-off required for protocol-critical signer rotation paths in ts-sdk#554).
🤖 Reviewed by Arkana
Rebuild vendor/*.tgz from arkade-os/ts-sdk@0e7378b: - fix(contractWatcher): recover promptly after subscription disruption (#564) - merge master into the PR branch The signer-rotation API and Contract type the wallet uses are unchanged. Verified against the new build: tsc, build (app + worker), unit tests, and a frozen CI=true install all pass.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit 19f4ed1f
Scope: Bump pinned ts-sdk ref from 41c8ee0 → 0e7378b.
What changed
package.json—arkadeSdk.refupdatedpnpm-lock.yaml— integrity hash updated for the rebuilt tarballvendor/.ref— ref marker updatedvendor/arkade-os-sdk.tgz— rebuilt tarball (binary, ~480 bytes larger)
What's in the new SDK ref
The delta between 41c8ee0 and 0e7378b is two commits:
664033ab—fix(contractWatcher): recover promptly after subscription disruption(ts-sdk#564). Adds reconnect resilience to the contract watcher (+29 lines incontractWatcher.ts, +29 lines test).0e7378b4— Merge oforigin/masterinto the signer-rotation feature branch. Catches the branch up.
Assessment
No wallet-side code changes. This is a clean SDK pin bump. The contractWatcher fix is a net positive — better reconnect handling reduces the chance of stale contract state in the wallet.
✅ No issues found.
🤖 Reviewed by Arkana
Rebuild vendor/*.tgz from arkade-os/ts-sdk@6c59b22. The new commits are e2e/regtest/CI only (drive signer rotation through the regtest CLI, run-on-all-PRs, regtest submodule bumps, merge #563) — no src changes, so the SDK build is unchanged. Verified against the new build: tsc, build (app + worker), unit tests, and a frozen CI=true install all pass.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit c3cc6162
Scope: Bump pinned ts-sdk ref from 0e7378b → 6c59b22.
Trivial change — ref bump + regenerated vendor/arkade-os-sdk.tgz. The commit message confirms the upstream delta is e2e/regtest/CI-only (no src/ changes), so the SDK build artifact is functionally identical. package.json, pnpm-lock.yaml, vendor/.ref all consistent.
Verdict: ✅ No issues.
🤖 Reviewed by Arkana
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit 0f6691d6
Scope: Bump pinned ts-sdk ref from 6c59b22 → ac4d3a5.
The upstream SDK change is a single-line version bump (buildVersion 0.9.7 → 0.9.9 in packages/ts-sdk/src/utils/fetch.ts). The wallet commit regenerates the vendored tarball and updates the lockfile integrity hash accordingly.
Files changed (all mechanical):
package.json— pinned ref updatedvendor/.ref— matches new refvendor/arkade-os-sdk.tgz— rebuilt tarballpnpm-lock.yaml— integrity hash updated
No protocol, security, or logic changes. LGTM.
🤖 Reviewed by Arkana
…n BUILD_VERSION_TOO_OLD
arkd's guard interceptors run outside its error-detail converter, so the REST rejection carries an empty details[] and the name only in the message. Bump the vendored SDK to 52236be (maybeArkError message-prefix fallback) and extract the min version from the message when metadata is absent, so the wallet shows the outdated-client prompt instead of 'Arkade server unreachable'.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commits a95ffb55..f367072f
Scope: 7 new commits — BUILD_VERSION_TOO_OLD detection, "Ark" → "Arkade" copy, vendored tarballs removed in favor of published SDK 0.4.35 / boltz-swap 0.3.40.
🔴 Blocking: Lockfile still references deleted vendor tarball
pnpm-lock.yaml is broken. package.json correctly points @arkade-os/sdk at 0.4.35, but the lockfile was not regenerated:
- Line 8 still has
overrides: '@arkade-os/sdk': file:./vendor/arkade-os-sdk.tgz(thepnpm.overrideswas removed frompackage.jsonbut the lockfile retains it) - Line 17-18 still resolves SDK as
specifier: file:vendor/arkade-os-sdk.tgz/version: file:vendor/arkade-os-sdk.tgz - Lines 254, 4271 still define the
@arkade-os/sdk@file:vendor/arkade-os-sdk.tgzpackage entry vendor/arkade-os-sdk.tgzwas deleted in commitf367072f
Result: pnpm install --frozen-lockfile (CI) will fail — the lockfile references a file that no longer exists.
Fix: Run pnpm install to regenerate the lockfile after the tarball removal, or do the two-step: first remove the pnpm.overrides + update package.json, then pnpm install, then commit the resulting lockfile.
⚠️ Minor: Inconsistent aspErrorText usage
aspErrorText() is used on 4 screens (Init, About, Server, Unavailable) but 5 others still use hardcoded 'Arkade server unreachable':
Vtxos.tsx:58Wallet/Index.tsx:94Notes/Form.tsx:32Notes/Redeem.tsx:32Send/Form.tsx:120, 458
If the server enforces a minimum build version mid-session (server upgrades while wallet is open), these screens will show "Arkade server unreachable" instead of the actionable "update your wallet" message. Not blocking, but worth making consistent.
✅ Good
- asp.ts:
ArkError+err.namecheck is well-structured. The regex fallback forminBuildVersionwhen metadata is missing is pragmatic given the arkd guard-interceptor ordering issue noted in the PR description. - asp.test.ts: Covers the three key paths (metadata present, metadata absent/regex, generic failure). Clean mock setup preserving
ArkErroras a real class forinstanceof. - No protocol-critical code touched — these are all UI/error-display changes.
🤖 Reviewed by Arkana
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commits a95ffb55..f367072f
Scope: 7 new commits — BUILD_VERSION_TOO_OLD detection, "Ark" → "Arkade" copy, vendored tarballs removed in favor of published SDK 0.4.35 / boltz-swap 0.3.40.
🔴 Blocking: Lockfile still references deleted vendor tarball
pnpm-lock.yaml is broken. package.json correctly points @arkade-os/sdk at 0.4.35, but the lockfile was not regenerated:
- Line 8 still has
overrides: '@arkade-os/sdk': file:./vendor/arkade-os-sdk.tgz(thepnpm.overrideswas removed frompackage.jsonbut the lockfile retains it) - Line 17-18 still resolves SDK as
specifier: file:vendor/arkade-os-sdk.tgz/version: file:vendor/arkade-os-sdk.tgz - Lines 254, 4271 still define the
@arkade-os/sdk@file:vendor/arkade-os-sdk.tgzpackage entry vendor/arkade-os-sdk.tgzwas deleted in commitf367072f
Result: pnpm install --frozen-lockfile (CI) will fail — the lockfile references a file that no longer exists.
Fix: Run pnpm install to regenerate the lockfile after the tarball removal, or do the two-step: first remove the pnpm.overrides + update package.json, then pnpm install, then commit the resulting lockfile.
⚠️ Minor: Inconsistent aspErrorText usage
aspErrorText() is used on 4 screens (Init, About, Server, Unavailable) but 5 others still use hardcoded 'Arkade server unreachable':
Vtxos.tsx:58Wallet/Index.tsx:94Notes/Form.tsx:32Notes/Redeem.tsx:32Send/Form.tsx:120, 458
If the server enforces a minimum build version mid-session (server upgrades while wallet is open), these screens will show "Arkade server unreachable" instead of the actionable "update your wallet" message. Not blocking, but worth making consistent.
✅ Good
- asp.ts:
ArkError+err.namecheck is well-structured. The regex fallback forminBuildVersionwhen metadata is missing is pragmatic given the arkd guard-interceptor ordering issue noted in the PR description. - asp.test.ts: Covers the three key paths (metadata present, metadata absent/regex, generic failure). Clean mock setup preserving
ArkErroras a real class forinstanceof. - No protocol-critical code touched — these are all UI/error-display changes.
🤖 Reviewed by Arkana
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commits 66a5c1f6, 2e4478c6
Both blocking issues from my previous review are resolved:
🔴→✅ Lockfile fixed (66a5c1f6): pnpm-lock.yaml now correctly resolves @arkade-os/sdk@0.4.35 and @arkade-os/boltz-swap@0.3.40 from the registry. No remaining references to the deleted vendor/*.tgz files. pnpm install --frozen-lockfile should pass.
aspErrorText consistency (2e4478c6): All 5 screens I flagged now use aspErrorText(aspInfo, fallback) instead of hardcoded strings:
Vtxos.tsx:58✅Wallet/Index.tsx:94✅Notes/Form.tsx:32✅Notes/Redeem.tsx:32✅Send/Form.tsx:120, 458✅
Send/Form.tsx correctly adds aspInfo.outdated to the useEffect dependency array (line 465) so the error text updates if the outdated state changes mid-session.
Cosmetic nit (non-blocking): Send/Form.tsx:463 still sets setLabel('Server unreachable') even when the issue is an outdated client. Could say info.outdated ? 'Update required' : 'Server unreachable' for consistency with the error message, but this is minor.
No protocol-critical code in this delta. LGTM.
🤖 Reviewed by Arkana
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commits 66a5c1f6, 2e4478c6
Both blocking issues from my previous review are resolved:
🔴→✅ Lockfile fixed (66a5c1f6): pnpm-lock.yaml now correctly resolves @arkade-os/sdk@0.4.35 and @arkade-os/boltz-swap@0.3.40 from the registry. No remaining references to the deleted vendor/*.tgz files. pnpm install --frozen-lockfile should pass.
aspErrorText consistency (2e4478c6): All 5 screens I flagged now use aspErrorText(aspInfo, fallback) instead of hardcoded strings:
Vtxos.tsx:58✅Wallet/Index.tsx:94✅Notes/Form.tsx:32✅Notes/Redeem.tsx:32✅Send/Form.tsx:120, 458✅
Send/Form.tsx correctly adds aspInfo.outdated to the useEffect dependency array (line 465) so the error text updates if the outdated state changes mid-session.
Cosmetic nit (non-blocking): Send/Form.tsx:463 still sets setLabel('Server unreachable') even when the issue is an outdated client. Could say info.outdated ? 'Update required' : 'Server unreachable' for consistency with the error message, but this is minor.
No protocol-critical code in this delta. LGTM.
🤖 Reviewed by Arkana
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Code Review — feat/signer-rotation-sdk-pr554
Overall this is a well-structured PR: good SDK vendoring approach (with clear cleanup plan), solid test coverage for the new paths, and the virtualized contract list is a nice perf improvement. That said, I'm flagging a few issues — one bug, one protocol-critical note, and some minor items.
🐛 Bug: Missing aspInfo.outdated in useEffect dependency arrays (3 files)
Send/Form.tsx and Server.tsx correctly added aspInfo.outdated to their useEffect deps after introducing aspErrorText(aspInfo, ...). Three other files did not:
src/screens/Wallet/Notes/Form.tsx—}, [aspInfo.unreachable])(line ~33 in diff)src/screens/Wallet/Notes/Redeem.tsx—}, [aspInfo.unreachable])(line ~32 in diff)src/screens/Settings/Vtxos.tsx—}, [aspInfo.unreachable])(line ~58 in diff)
All three call aspErrorText(aspInfo, ...) which reads aspInfo.outdated, but the effect won't re-fire if outdated changes while unreachable stays true. Scenario: server flips from BUILD_VERSION_TOO_OLD to a generic network error (or vice versa) — unreachable stays true both times, but the error text is stale. Fix: add aspInfo.outdated to each deps array, matching what Server.tsx and Send/Form.tsx already do.
This is also an eslint-plugin-react-hooks/exhaustive-deps violation.
⚠️ Protocol-Critical: Signer rotation classification — requesting human review
The contractSignerStatus() function in Contracts.tsx and the DeprecatedSignerBadge component surface signer rotation status to users. While this is display-only (the actual migration logic lives in the SDK's SettlementConfig.deprecatedSignerMigration), incorrect classification could mislead users about the safety of their VTXOs — e.g., showing "active signer" when the signer is actually deprecated and past cutoff, potentially causing users to delay a time-critical unilateral exit.
The classification logic delegates correctly to classifyAgainstSignerSet() from the SDK, and the badge mapping (EXPIRED → red "past cutoff", MIGRATABLE/DUE_NOW → orange "deprecated signer", everything else → no badge) is sound.
Per protocol rules: this touches signer rotation and implicitly affects user decisions about exit paths. Requesting explicit human sign-off.
Minor observations (non-blocking)
-
Silent catch blocks —
onchainTaprootAddress,contractSignerStatus, and thesignerSetFromInfomemo all silently swallow errors with barecatch {}. Consider at least aconsole.debugin dev mode so misconfigurations don't fail silently during development. -
onchainTaprootAddresscorrectness ✅ — Verified: the P2TR scriptPubKey check (/^5120[0-9a-f]{64}$/i), witness program extraction, HRP mapping (bc/tb/bcrt), and bech32m encoding with witness version 1 are all correct. -
Virtualizer key choice — Using
contract.scriptas the virtualizer key is a good call (avoids address collisions on boarding contracts). The comment explaining this is appreciated. -
SDK error boundary — The
getAspInfocatch path correctly checkserr instanceof ArkError && err.name === BUILD_VERSION_TOO_OLD, and the fallback regexerr.message.match(/>=\s*v?([\d.]+)/)handles the rc.1 case where metadata is absent. The test covers both paths (with and without metadata). Solid. -
Trailing period inconsistency —
Unavailable.tsxpasses'Arkade server unreachable.'(with period) while every other caller passes'Arkade server unreachable'(without). Pre-existing, not introduced by this PR, but worth a cleanup pass.
Verdict
Request changes for the missing deps bug. The protocol-critical signer display needs a human to sign off.
The vendoring approach, SDK consumption, error handling, and UI improvements are all well-done. Fix the three dep arrays and this is ready for human approval.
🤖 Reviewed by Arkana
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/Wallet/Unavailable.tsx (1)
17-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEffect is not reactive to ASP status changes.
Line 18 reads
aspInfo.unreachable/aspInfo.outdated, but the effect runs only once ([]). If ASP status changes after mount, the message can stay stale and miss the outdated-wallet text.Suggested fix
- useEffect(() => { - if (aspInfo.unreachable) return setError(aspErrorText(aspInfo, 'Arkade server unreachable.')) + useEffect(() => { + if (aspInfo.unreachable) { + setError(aspErrorText(aspInfo, 'Arkade server unreachable.')) + return + } detectJSCapabilities() .then((result) => { if (result.isSupported) return // Use specific error message or fallback to iOS/generic message setError(result.errorMessage || getRestrictedEnvironmentMessage(isIOS())) }) .catch(() => { setError(getRestrictedEnvironmentMessage(isIOS())) }) - }, []) + }, [aspInfo.unreachable, aspInfo.outdated])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/Wallet/Unavailable.tsx` around lines 17 - 28, The useEffect hook starting with the aspInfo.unreachable check depends on the aspInfo object but has an empty dependency array, causing the effect to run only once at mount and miss updates to ASP status. Add aspInfo to the dependency array of the useEffect call to make the effect reactive to changes in the aspInfo object, ensuring the error message updates when ASP status changes from reachable to unreachable or when the outdated state updates.
🧹 Nitpick comments (1)
src/screens/Settings/Contracts.tsx (1)
147-166: 💤 Low valueConsider adding keyboard accessibility to Chip.
The
Chipcomponent uses a<div>withonClickbut lacksrole="button",tabIndex={0}, andonKeyDownhandling for Enter/Space. Users relying on keyboard navigation cannot activate these filter pills.♿ Proposed accessibility fix
function Chip({ label, active, onClick }: { label: string; active: boolean; onClick: () => void }) { return ( <div onClick={onClick} + onKeyDown={(e) => (e.key === 'Enter' || e.key === ' ') && (e.preventDefault(), onClick())} + role="button" + tabIndex={0} style={{ padding: '0.25rem 0.6rem', borderRadius: '999px', cursor: 'pointer',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/Settings/Contracts.tsx` around lines 147 - 166, The Chip component currently uses a div with onClick but lacks keyboard accessibility attributes and handlers, preventing keyboard users from interacting with the filter pills. Add role="button" to the div to identify it as an interactive element, add tabIndex={0} to make it focusable via keyboard navigation, and implement an onKeyDown handler that triggers the onClick callback when the Enter or Space keys are pressed. This ensures the component is fully accessible to keyboard and screen reader users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 66: The prepare:sdk script entry is defined in package.json but the
required arkadeSdk configuration object is missing. Add an arkadeSdk
configuration block to package.json with the required repo and ref properties
that the prepare-arkade-sdk.js script expects (as checked on lines 28-31 of that
script), or alternatively remove the prepare:sdk script entry from package.json
until the necessary configuration is added.
In `@scripts/prepare-arkade-sdk.js`:
- Line 107: Add the --frozen-lockfile flag to the pnpm install command in the
run() function call to ensure deterministic vendor artifacts. Modify the pnpm
install invocation to include --frozen-lockfile, which will fail fast if the
lockfile and package manifests diverge, preventing dependency-resolution drift
in the vendoring pipeline.
- Around line 75-90: The git checkout logic in the
`fs.existsSync(path.join(srcDir, '.git'))` block verifies that the commit hash
matches `cfg.ref` but does not verify that the working tree is clean. Add a
check using git status (e.g., `git status --porcelain`) to detect if there are
uncommitted changes or untracked files in the working tree. If the working tree
is dirty, either clean it automatically, fail with an error message, or warn and
proceed based on your project's reproducibility requirements. This check should
be performed after confirming the .git directory exists and before proceeding
with the fetch or checkout operations to prevent modified files from being
included in the generated tarballs.
In `@src/screens/Settings/Vtxos.tsx`:
- Around line 57-59: The useEffect hooks in three files are calling
aspErrorText(...) which depends on aspInfo.outdated, but the dependency arrays
only include aspInfo.unreachable. This causes the effect to not re-run when only
the outdated state changes, leaving stale error text. Add aspInfo.outdated to
the dependency array in the useEffect hook at src/screens/Settings/Vtxos.tsx
lines 57-59, the useEffect hook at src/screens/Wallet/Notes/Form.tsx lines
32-34, and the useEffect hook at src/screens/Wallet/Notes/Redeem.tsx lines 31-33
so the error message updates whenever either unreachable or outdated state
changes.
In `@src/screens/Wallet/Send/Form.tsx`:
- Around line 457-465: The conditional error clearing logic on line 460 only
removes the error if it matches the current errTxt (the unreachable message).
When transitioning from an outdated state to a reachable state, the outdated
message persists because it doesn't match the unreachable errTxt. Fix this by
unconditionally clearing the error to an empty string when aspInfo.unreachable
is false, rather than conditionally checking if prev === errTxt. This ensures
both ASP-unavailable message variants (unreachable and outdated) are properly
cleared when the server becomes reachable. Additionally, update the dependency
array to [aspInfo.unreachable] since that is the primary state being checked in
the if condition.
---
Outside diff comments:
In `@src/screens/Wallet/Unavailable.tsx`:
- Around line 17-28: The useEffect hook starting with the aspInfo.unreachable
check depends on the aspInfo object but has an empty dependency array, causing
the effect to run only once at mount and miss updates to ASP status. Add aspInfo
to the dependency array of the useEffect call to make the effect reactive to
changes in the aspInfo object, ensuring the error message updates when ASP
status changes from reachable to unreachable or when the outdated state updates.
---
Nitpick comments:
In `@src/screens/Settings/Contracts.tsx`:
- Around line 147-166: The Chip component currently uses a div with onClick but
lacks keyboard accessibility attributes and handlers, preventing keyboard users
from interacting with the filter pills. Add role="button" to the div to identify
it as an interactive element, add tabIndex={0} to make it focusable via keyboard
navigation, and implement an onKeyDown handler that triggers the onClick
callback when the Enter or Space keys are pressed. This ensures the component is
fully accessible to keyboard and screen reader users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2756b74-2caa-41b9-9c2f-cf3db9d08596
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.gitignorepackage.jsonscripts/prepare-arkade-sdk.jssrc/lib/asp.tssrc/providers/asp.tsxsrc/screens/Init/Init.tsxsrc/screens/Settings/About.tsxsrc/screens/Settings/Contracts.tsxsrc/screens/Settings/Server.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Index.tsxsrc/screens/Wallet/Notes/Form.tsxsrc/screens/Wallet/Notes/Redeem.tsxsrc/screens/Wallet/Send/Form.tsxsrc/screens/Wallet/Unavailable.tsxsrc/test/lib/asp.test.tssrc/test/screens/settings/contracts.test.tsxvitest.config.ts
…1y, drop orphaned SDK vendoring script
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit 4ef0a6d1
Three changes, all clean:
1. Orphaned vendor script cleanup ✅
Removes scripts/prepare-arkade-sdk.js, the prepare:sdk script entry, .gitignore entries for .arkade-sdk-src/ and vendor/, and the vitest exclude patterns. All of these became dead code after 54916bdf switched to published SDK 0.4.35. No issues.
2. Chip keyboard a11y ✅
Contracts.tsx:150-158 — Adds role='button', tabIndex={0}, and Enter/Space onKeyDown handler. Standard pattern, correct.
3. aspInfo.outdated dependency arrays ✅
Vtxos.tsx:59, Notes/Form.tsx:34, Notes/Redeem.tsx:33, Unavailable.tsx:27 — Adding aspInfo.outdated to useEffect deps ensures the error message re-evaluates when the outdated flag flips, not just when unreachable changes. Without this, a transition from generic-unreachable to outdated (or vice versa) while unreachable stays true would leave stale text. Correct fix.
Send/Form.tsx:462-465 — The error-clearing logic now also clears the outdated-client message when the server becomes reachable. The approach of synthesizing aspErrorText({ ...aspInfo, outdated: true }, errTxt) to get the outdated string is slightly indirect but correct and forward-compatible with any future aspErrorText changes.
No protocol-critical changes. All modifications are UI error display and cleanup.
🤖 Reviewed by Arkana
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — new commit 4ef0a6d1
Three changes, all clean:
1. Orphaned vendor script cleanup ✅
Removes scripts/prepare-arkade-sdk.js, the prepare:sdk script entry, .gitignore entries for .arkade-sdk-src/ and vendor/, and the vitest exclude patterns. All of these became dead code after 54916bdf switched to published SDK 0.4.35. No issues.
2. Chip keyboard a11y ✅
Contracts.tsx:150-158 — Adds role='button', tabIndex={0}, and Enter/Space onKeyDown handler. Standard pattern, correct.
3. aspInfo.outdated dependency arrays ✅
Vtxos.tsx:59, Notes/Form.tsx:34, Notes/Redeem.tsx:33, Unavailable.tsx:27 — Adding aspInfo.outdated to useEffect deps ensures the error message re-evaluates when the outdated flag flips, not just when unreachable changes. Without this, a transition from generic-unreachable to outdated (or vice versa) while unreachable stays true would leave stale text. Correct fix.
Send/Form.tsx:462-465 — The error-clearing logic now also clears the outdated-client message when the server becomes reachable. The approach of synthesizing aspErrorText({ ...aspInfo, outdated: true }, errTxt) to get the outdated string is slightly indirect but correct and forward-compatible with any future aspErrorText changes.
No protocol-critical changes. All modifications are UI error display and cleanup.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/Wallet/Unavailable.tsx (1)
17-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale async capability checks from overwriting ASP outage/outdated errors.
detectJSCapabilities()can resolve afteraspInfo.unreachable/outdatedflips, and thensetError(...)in the old promise chain can replace the newer server/outdated message. Please guard the async path with effect cleanup cancellation.Suggested fix
useEffect(() => { + let cancelled = false + if (aspInfo.unreachable) return setError(aspErrorText(aspInfo, 'Arkade server unreachable.')) detectJSCapabilities() .then((result) => { + if (cancelled) return if (result.isSupported) return // Use specific error message or fallback to iOS/generic message setError(result.errorMessage || getRestrictedEnvironmentMessage(isIOS())) }) .catch(() => { + if (cancelled) return setError(getRestrictedEnvironmentMessage(isIOS())) }) + return () => { + cancelled = true + } }, [aspInfo.unreachable, aspInfo.outdated])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/Wallet/Unavailable.tsx` around lines 17 - 28, The useEffect hook in the Unavailable component has a race condition where the async detectJSCapabilities() promise can resolve after aspInfo.unreachable or aspInfo.outdated values change, causing the stale promise to overwrite the newer server error message with setError(). To fix this, add a cleanup mechanism using an AbortController or a mounted flag. Before calling setError() in the .then() and .catch() handlers of detectJSCapabilities(), check if the flag is still valid (or if the signal is not aborted), and only proceed if the effect is still active. In the cleanup function returned from useEffect, either abort the controller or set the flag to false to prevent stale callbacks from executing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/screens/Wallet/Unavailable.tsx`:
- Around line 17-28: The useEffect hook in the Unavailable component has a race
condition where the async detectJSCapabilities() promise can resolve after
aspInfo.unreachable or aspInfo.outdated values change, causing the stale promise
to overwrite the newer server error message with setError(). To fix this, add a
cleanup mechanism using an AbortController or a mounted flag. Before calling
setError() in the .then() and .catch() handlers of detectJSCapabilities(), check
if the flag is still valid (or if the signal is not aborted), and only proceed
if the effect is still active. In the cleanup function returned from useEffect,
either abort the controller or set the flag to false to prevent stale callbacks
from executing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f746e372-7cb1-4736-8199-c04f3fc9f972
📒 Files selected for processing (7)
package.jsonsrc/screens/Settings/Contracts.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Notes/Form.tsxsrc/screens/Wallet/Notes/Redeem.tsxsrc/screens/Wallet/Send/Form.tsxsrc/screens/Wallet/Unavailable.tsx
💤 Files with no reviewable changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/screens/Wallet/Notes/Redeem.tsx
- src/screens/Settings/Vtxos.tsx
- src/screens/Wallet/Notes/Form.tsx
- src/screens/Wallet/Send/Form.tsx
- src/screens/Settings/Contracts.tsx
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — merge commit 8f3c9e8d
Scope: Merge of origin/master into feat/signer-rotation-sdk-pr554, bringing in two already-merged PRs:
89e75ac7— Branta SDK upgrade 0.0.9 → 3.1.3 (#673)cd5931eb— LNURL fixed-amount field population fix (#671)
Findings: None. The merge is clean:
- No conflict markers in the combined
Send/Form.tsx - The
aspErrorTextintegration from4ef0a6d1(this PR) is intact alongside both merged changes - LNURL fix correctly uses
setState()(notsetAmount) so the amount input reflects fixed-amount LNURLs — consistent with this PR'ssetStatepattern - Branta v3.1.3 API (
BrantaService,Payment,PrivacyMode.Strict,getPaymentsByQrCode/getPayments) is properly wired with theme-aware logo selection and theverifyUrlanchor - Test coverage for the LNURL regression is included
package.jsonand lockfile are consistent
No protocol-critical code touched in this merge. Approval stands.
🤖 Reviewed by Arkana
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — merge commit 8f3c9e8d
Scope: Merge of origin/master into feat/signer-rotation-sdk-pr554, bringing in two already-merged PRs:
89e75ac7— Branta SDK upgrade 0.0.9 → 3.1.3 (#673)cd5931eb— LNURL fixed-amount field population fix (#671)
Findings: None. The merge is clean:
- No conflict markers in the combined
Send/Form.tsx - The
aspErrorTextintegration from4ef0a6d1(this PR) is intact alongside both merged changes - LNURL fix correctly uses
setState()(notsetAmount) so the amount input reflects fixed-amount LNURLs — consistent with this PR'ssetStatepattern - Branta v3.1.3 API (
BrantaService,Payment,PrivacyMode.Strict,getPaymentsByQrCode/getPayments) is properly wired with theme-aware logo selection and theverifyUrlanchor - Test coverage for the LNURL regression is included
package.jsonand lockfile are consistent
No protocol-critical code touched in this merge. Approval stands.
🤖 Reviewed by Arkana
What
Makes the wallet build against ts-sdk#554 (arkd signer-rotation support) and surfaces deprecated signers per contract.
1. Consume the unpublished SDK PR via vendored tarballs
ts-sdk is a pnpm monorepo built with
tsupthat ships onlydist/, has nopreparescript, keeps its build toolchain at the repo root, and linksboltz-swap → sdkwithworkspace:*— so the packages can't be installed straight from a git ref.scripts/prepare-arkade-sdk.jsclones the ref pinned inpackage.json("arkadeSdk", currently52236be), builds both packages, and packs them intovendor/*.tgz.package.jsonreferences them viafile:, plus apnpm.overridesentry so boltz-swap'sworkspace:*sdk dependency resolves to the same build (one SDK instance, not a mix with the published0.4.34).To bump the pinned commit:
pnpm prepare:sdk(optionallyARKADE_SDK_DIR=/path/to/ts-sdkto reuse a checkout), then commit the regeneratedvendor/*.tgz+pnpm-lock.yaml.2. Flag deprecated-signer contracts (dev mode only)
Settings → Advanced → Contracts(already dev-gated) classifies each contract'sparams.serverPubKeyagainst the operator's advertised signer set via the SDK'ssignerSetFromInfo/classifyAgainstSignerSet:deprecated signer— minted under a deprecated signer, cutoff not yet passed.deprecated signer · past cutoff(red) — the cooperative-migration cutoff has passed.Background auto-migration (
SettlementConfig.deprecatedSignerMigration, defaulttrue) is unchanged.3. Clearer message when the client is too old
When the server enforces a minimum
X-Build-Versionand the wallet is below it, arkd rejects every call (includinggetInfo) with aBUILD_VERSION_TOO_OLDerror — so the wallet previously showed a misleading "server unreachable".getAspInfonow detects the typedArkErrorand surfaces "Your wallet is outdated and needs to be updated to be compatible with the latest Arkade version." across the Unavailable / Init / About / Server screens.Note: on arkd
v0.9.9-rc.1the build-version and digest guard interceptors run outside the error-detail converter, so their REST errors arrive with an emptydetails[]and the structured name only in the message string. The SDK bump (52236be) makesmaybeArkErrorfall back to parsing theNAME (code): messageprefix, which also repairsDIGEST_MISMATCHdetection (mid-session signer rotation) against rc.1. Root-cause fix belongs in arkd (reordererrorConverterto wrap the guards).Verification
tsc --noEmit,pnpm lint, prettier (changed files) — cleanpnpm build(app + service-worker bundle) — builds against the PR SDKpnpm test:unit— 276 passed, incl. badge tests (past-cutoff / deprecated / active-signer) and outdated-client tests (getAspInfo/aspErrorText)CI=true pnpm install(frozen) resolves the committed tarballsMerge checklist
@arkade-os/sdk+@arkade-os/boltz-swappublished to npmscripts/prepare-arkade-sdk.js, thearkadeSdkfield, thepnpm.overrides, the committedvendor/tarballs, and the.arkade-sdk-srcignore; point the two deps at the published versionsSummary by CodeRabbit
Release Notes