Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdd delegator support and runtime restart for the service-worker wallet (delegatorUrl, delegateVtxos, restartWallet), normalize compressed pubkeys to x-only, update env/config keys and workspace/dependency metadata, and propagate delegatorUrl through wallet initialization and retry logic. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant App
participant WalletProvider
participant ServiceWorkerWallet
participant DelegatorAPI
end
App->>WalletProvider: request wallet init / restart (delegateEnabled?, delegatorUrl)
WalletProvider->>ServiceWorkerWallet: initSvcWorkerWallet(..., delegatorUrl)
WalletProvider->>ServiceWorkerWallet: setup(..., delegatorUrl)
alt delegation enabled
ServiceWorkerWallet->>DelegatorAPI: delegateVtxos requests using delegatorUrl
DelegatorAPI-->>ServiceWorkerWallet: delegation responses
else delegation disabled
ServiceWorkerWallet->>ServiceWorkerWallet: renewCoins / normal startup tasks
end
ServiceWorkerWallet-->>WalletProvider: setup complete / error
WalletProvider-->>App: wallet ready or retry flow (backoff, retry with delegatorUrl)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Deploying wallet-bitcoin with
|
| Latest commit: |
cb39c10
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1c12a769.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://delegator.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
cb39c10
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a8fdf0ba.arkade-wallet.pages.dev |
| Branch Preview URL: | https://delegator.arkade-wallet.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/wallet.tsx (1)
234-240:delegatorUrlis not passed in the retry call.When retrying after a timeout,
delegatorUrlis not forwarded to the recursive call, causing it to fall back to the default value instead of preserving the originally provided URL.🐛 Proposed fix
return initSvcWorkerWallet({ arkServerUrl, esploraUrl, privateKey, retryCount: retryCount + 1, maxRetries, + delegatorUrl, })
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 8: The package.json entry for the dependency "@arkade-os/sdk":
"github:louisinger/wallet-sdk#delegator" points to a mutable personal-branch;
replace it with a stable reference before merging by either (a) switching to the
published npm version of `@arkade-os/sdk` that contains the delegator changes
(e.g., "@arkade-os/sdk": "x.y.z"), or (b) pinning the GitHub reference to an
immutable commit SHA (e.g., "github:louisinger/wallet-sdk#<commit-sha>") and, if
possible, use the organization’s repository instead of a personal fork to ensure
reproducible builds.
In `@src/providers/wallet.tsx`:
- Line 167: The default delegatorUrl is hardcoded to Mutiny testnet; update the
code that sets delegatorUrl to derive it from the active network instead of a
fixed string: use aspInfo.network (or the same network source used for
esploraUrl) and compute the delegator base via a helper similar to
getRestApiExplorerURL (or add a getDelegatorURL(network) function) so the
delegatorUrl choice follows the network/environment and works for mainnet and
other networks.
Kukks
left a comment
Kukks
left a comment
There was a problem hiding this comment.
lets turn this into a releasable change please
51185bb to
451ff4a
Compare
|
@CodeRabbit please review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/lib/asp.ts (1)
295-304: Small cleanup: compute delegate pubkey once before filtering.This avoids repeated normalization work and improves readability in the filter pipeline.
♻️ Proposed cleanup
const delegateInfo = await dm.getDelegateInfo() + const delegateInfoPubKey = toXOnlyHex(delegateInfo.pubkey) const vtxosToDelegate = contractWithVtxos .filter(({ contract, vtxos }) => { if (vtxos.length === 0) return false const contractParams = DelegateContractHandler.deserializeParams(contract.params) const contractDelegatePubKey = hex.encode(contractParams.delegatePubKey) // x-only (32 bytes) - const delegateInfoPubKey = toXOnlyHex(delegateInfo.pubkey) return contractDelegatePubKey === delegateInfoPubKey }) - .flatMap((_) => _.vtxos) + .flatMap(({ vtxos }) => vtxos)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/asp.ts` around lines 295 - 304, Compute delegateInfoPubKey once before building vtxosToDelegate to avoid repeated normalization inside the filter: call dm.getDelegateInfo() as you already do, then compute const delegateInfoPubKey = toXOnlyHex(delegateInfo.pubkey) (or the normalized x-only hex) once, and inside the vtxosToDelegate filter compare contractDelegatePubKey === delegateInfoPubKey; keep using DelegateContractHandler.deserializeParams and hex.encode for contract.params as before but remove repeated toXOnlyHex(delegateInfo.pubkey) calls from the filter.src/screens/Settings/Delegates.tsx (1)
199-204: Prefer in-app wallet restart over full page reload.This works, but
window.location.reload()is heavier than needed. Since wallet restart support exists, this toggle flow can reinitialize wallet services without hard-refreshing the app.♻️ Proposed refactor
export default function Delegates() { const { aspInfo } = useContext(AspContext) const { goBack } = useContext(OptionsContext) const { config, updateConfig } = useContext(ConfigContext) + const { restartWallet } = useContext(WalletContext) @@ const handleToggle = async () => { const nextDelegate = !config.delegate await updateConfig({ ...config, delegate: nextDelegate }) - // Full page reload ensures service worker and wallet are re-instantiated with the new delegator setting. - window.location.reload() + await restartWallet(nextDelegate) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Settings/Delegates.tsx` around lines 199 - 204, The handleToggle function currently calls window.location.reload() after updateConfig; replace the hard reload with the in-app wallet restart flow: after awaiting updateConfig({ ...config, delegate: nextDelegate }) call the existing wallet/service restart helper (e.g., restartWallet(), reinitializeWalletServices(), or similar service-worker reinit function used elsewhere) and any necessary re-subscription code so the wallet and service worker pick up the new delegator setting without a full page reload; locate handleToggle in Delegates.tsx and wire the canonical restart helper used by your wallet module instead of window.location.reload().src/providers/wallet.tsx (1)
274-281: Inconsistent error handling between delegation and renewal paths.
delegateVtxoslogs errors viaconsole.error, butrenewCoinsuses an empty.catch(() => {})that silently swallows failures. Consider consistent error handling for both paths.♻️ Suggested improvement
// delegate or renew expiring coins on startup if (config.delegate) { delegateVtxos(svcWallet).catch((e) => { console.error('Error delegating coins', e) }) } else { - renewCoins(svcWallet, aspInfo.dust, wallet.thresholdMs).catch(() => {}) + renewCoins(svcWallet, aspInfo.dust, wallet.thresholdMs).catch((e) => { + console.error('Error renewing coins', e) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/wallet.tsx` around lines 274 - 281, The renewal path is silently swallowing errors — make error handling consistent with the delegation path by catching and logging errors from renewCoins(svcWallet, aspInfo.dust, wallet.thresholdMs) instead of using .catch(() => {}); update the .catch to log a descriptive error (e.g., "Error renewing coins") along with the caught error object and keep behavior consistent with the delegateVtxos(svcWallet) branch that currently uses console.error; ensure you reference config.delegate to preserve the branch logic and svcWallet/aspInfo.dust/wallet.thresholdMs parameters when implementing the logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/keys.ts`:
- Around line 8-12: The toXOnlyHex function currently only strips a lowercase
'0x' prefix and returns mixed-case hex, which can cause mismatches; update the
implementation to normalize the incoming pubkey by trimming whitespace,
lowercasing it, and then remove a case-insensitive '0x' prefix (handle '0X' too)
before applying the x-only logic: operate on the normalized lowercase string and
use the same length/checks on that value (reference to toXOnlyHex, pubkey,
normalized).
In `@src/providers/wallet.tsx`:
- Around line 336-349: The restartWallet function currently uses an unsafe cast
`(svcWallet.identity as any).toHex()`; replace this with a type-safe approach by
narrowing svcWallet.identity to the known SingleKey shape (e.g., assert or guard
that identity is SingleKey and then call toHex()) or define an interface/type
with toHex(): string and cast to that type after the `'toHex' in
svcWallet.identity` check; ensure the code uses the narrowed/typed identity when
calling toHex before passing privateKey into initSvcWorkerWallet so TypeScript
no longer requires `as any` and the runtime guard aligns with the static type
(refer to restartWallet, svcWallet.identity, toHex, SingleKey, and
initSvcWorkerWallet).
---
Nitpick comments:
In `@src/lib/asp.ts`:
- Around line 295-304: Compute delegateInfoPubKey once before building
vtxosToDelegate to avoid repeated normalization inside the filter: call
dm.getDelegateInfo() as you already do, then compute const delegateInfoPubKey =
toXOnlyHex(delegateInfo.pubkey) (or the normalized x-only hex) once, and inside
the vtxosToDelegate filter compare contractDelegatePubKey ===
delegateInfoPubKey; keep using DelegateContractHandler.deserializeParams and
hex.encode for contract.params as before but remove repeated
toXOnlyHex(delegateInfo.pubkey) calls from the filter.
In `@src/providers/wallet.tsx`:
- Around line 274-281: The renewal path is silently swallowing errors — make
error handling consistent with the delegation path by catching and logging
errors from renewCoins(svcWallet, aspInfo.dust, wallet.thresholdMs) instead of
using .catch(() => {}); update the .catch to log a descriptive error (e.g.,
"Error renewing coins") along with the caught error object and keep behavior
consistent with the delegateVtxos(svcWallet) branch that currently uses
console.error; ensure you reference config.delegate to preserve the branch logic
and svcWallet/aspInfo.dust/wallet.thresholdMs parameters when implementing the
logging.
In `@src/screens/Settings/Delegates.tsx`:
- Around line 199-204: The handleToggle function currently calls
window.location.reload() after updateConfig; replace the hard reload with the
in-app wallet restart flow: after awaiting updateConfig({ ...config, delegate:
nextDelegate }) call the existing wallet/service restart helper (e.g.,
restartWallet(), reinitializeWalletServices(), or similar service-worker reinit
function used elsewhere) and any necessary re-subscription code so the wallet
and service worker pick up the new delegator setting without a full page reload;
locate handleToggle in Delegates.tsx and wire the canonical restart helper used
by your wallet module instead of window.location.reload().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 776c7c7a-2da1-4edf-b48b-611be51a60ea
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
README.mdpackage.jsonpnpm-workspace.yamlpublic/wallet-service-worker.mjssrc/lib/asp.tssrc/lib/backup.tssrc/lib/constants.tssrc/lib/keys.tssrc/lib/nostr.tssrc/providers/wallet.tsxsrc/screens/Settings/Delegates.tsxsrc/test/screens/mocks.tssrc/vite-env.d.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- pnpm-workspace.yaml
- package.json
| const normalized = pubkey.startsWith('0x') ? pubkey.slice(2) : pubkey | ||
| if (normalized.length === 66 && (normalized.startsWith('02') || normalized.startsWith('03'))) { | ||
| return normalized.slice(2) | ||
| } | ||
| return normalized |
There was a problem hiding this comment.
Normalize hex prefix/case defensively in toXOnlyHex.
0X-prefixed keys are not handled right now, and mixed-case hex can create false mismatches in direct string comparisons.
🛠️ Proposed fix
export const toXOnlyHex = (pubkey: string): string => {
if (!pubkey) return pubkey
- // strip optional 0x prefix
- const normalized = pubkey.startsWith('0x') ? pubkey.slice(2) : pubkey
+ // strip optional 0x/0X prefix and canonicalize case
+ const normalized = pubkey.replace(/^0x/i, '').toLowerCase()
if (normalized.length === 66 && (normalized.startsWith('02') || normalized.startsWith('03'))) {
return normalized.slice(2)
}
return normalized
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const normalized = pubkey.startsWith('0x') ? pubkey.slice(2) : pubkey | |
| if (normalized.length === 66 && (normalized.startsWith('02') || normalized.startsWith('03'))) { | |
| return normalized.slice(2) | |
| } | |
| return normalized | |
| export const toXOnlyHex = (pubkey: string): string => { | |
| if (!pubkey) return pubkey | |
| // strip optional 0x/0X prefix and canonicalize case | |
| const normalized = pubkey.replace(/^0x/i, '').toLowerCase() | |
| if (normalized.length === 66 && (normalized.startsWith('02') || normalized.startsWith('03'))) { | |
| return normalized.slice(2) | |
| } | |
| return normalized | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/keys.ts` around lines 8 - 12, The toXOnlyHex function currently only
strips a lowercase '0x' prefix and returns mixed-case hex, which can cause
mismatches; update the implementation to normalize the incoming pubkey by
trimming whitespace, lowercasing it, and then remove a case-insensitive '0x'
prefix (handle '0X' too) before applying the x-only logic: operate on the
normalized lowercase string and use the same length/checks on that value
(reference to toXOnlyHex, pubkey, normalized).
| const restartWallet = async (delegateEnabled = config.delegate) => { | ||
| if (!svcWallet) return | ||
| const privateKey = 'toHex' in svcWallet.identity ? (svcWallet.identity as any).toHex() : undefined | ||
| if (!privateKey) throw new Error('Unable to reinitialize wallet without private key') | ||
| const arkServerUrl = aspInfo.url | ||
| const esploraUrl = getRestApiExplorerURL(aspInfo.network as NetworkName) ?? '' | ||
| const delegatorUrl = delegateEnabled ? defaultDelegate().url : undefined | ||
| await initSvcWorkerWallet({ | ||
| privateKey, | ||
| arkServerUrl, | ||
| esploraUrl, | ||
| delegatorUrl, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'restartWallet' --type tsx --type ts -g '!*.d.ts'Repository: arkade-os/wallet
Length of output: 87
🏁 Script executed:
# Check the wallet.tsx file to understand the context and svcWallet type
fd 'wallet.tsx' -t f | head -5Repository: arkade-os/wallet
Length of output: 105
🏁 Script executed:
# Look for the identity type definition
rg -n 'svcWallet.*=' --type tsx --type ts -A 2 -B 2 | head -50Repository: arkade-os/wallet
Length of output: 87
🏁 Script executed:
rg -n 'restartWallet' --type ts -g '!*.d.ts' -g '!node_modules'Repository: arkade-os/wallet
Length of output: 442
🏁 Script executed:
# Find and examine the wallet.tsx file to understand svcWallet and identity type
cat -n src/providers/wallet.tsx | head -100Repository: arkade-os/wallet
Length of output: 4625
🏁 Script executed:
# Check Delegates.tsx for comparison
fd 'Delegates.tsx' -t fRepository: arkade-os/wallet
Length of output: 94
🏁 Script executed:
# Get more context around the restartWallet function
cat -n src/providers/wallet.tsx | sed -n '330,360p'Repository: arkade-os/wallet
Length of output: 1500
🏁 Script executed:
# Check the Delegates.tsx file to see how delegate toggle is handled
cat -n src/screens/Settings/Delegates.tsxRepository: arkade-os/wallet
Length of output: 10903
🏁 Script executed:
# Search for where restartWallet is actually called/used in the application
rg -n 'restartWallet\(' --type ts -A 2 -B 2Repository: arkade-os/wallet
Length of output: 42
🏁 Script executed:
# Check what ServiceWorkerWallet type looks like and what identity is
rg -n 'ServiceWorkerWallet' --type ts -A 5 -B 5 | head -80Repository: arkade-os/wallet
Length of output: 4745
🏁 Script executed:
# Look for the identity type more specifically
rg -n 'identity.*:' --type ts | grep -i wallet | head -20Repository: arkade-os/wallet
Length of output: 269
🏁 Script executed:
# Check if restartWallet is passed to any handler or used indirectly
rg -n 'restartWallet' --type ts -B 3 -A 3Repository: arkade-os/wallet
Length of output: 2344
🏁 Script executed:
# Check if SingleKey type has proper type definitions in the SDK
rg -n 'SingleKey' --type ts -B 2 -A 2 | head -40Repository: arkade-os/wallet
Length of output: 1336
🏁 Script executed:
# Look at the actual structure to understand what toHex should be
cat -n src/providers/wallet.tsx | sed -n '200,215p'Repository: arkade-os/wallet
Length of output: 724
🏁 Script executed:
# Check what the comment says more clearly and look for usage pattern
cat -n src/providers/wallet.tsx | sed -n '195,215p'Repository: arkade-os/wallet
Length of output: 866
🏁 Script executed:
# Check what the initSvcWorkerWallet signature expects
rg -n 'initSvcWorkerWallet' --type ts -B 2 -A 8 | head -60Repository: arkade-os/wallet
Length of output: 2462
🏁 Script executed:
# Verify SingleKey type has toHex method by searching the SDK or usage patterns
rg -n '\.toHex\(\)' --type ts | head -20Repository: arkade-os/wallet
Length of output: 191
Remove unsafe type assertion or add proper type definition for identity.toHex().
Line 338 uses as any to bypass type checking. Since identity is always a SingleKey instance (created at line 207), consider either:
- Explicitly type the narrowed identity as
SingleKeyafter the check - Use a type-safe pattern like
(svcWallet.identity as { toHex(): string }).toHex()
The runtime check 'toHex' in svcWallet.identity ensures safety in practice, but the unsafe cast should be avoided. Note that Delegates.tsx intentionally uses window.location.reload() instead of restartWallet to ensure service worker re-instantiation; restartWallet is designed for in-place wallet reinitialization without a full reload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/providers/wallet.tsx` around lines 336 - 349, The restartWallet function
currently uses an unsafe cast `(svcWallet.identity as any).toHex()`; replace
this with a type-safe approach by narrowing svcWallet.identity to the known
SingleKey shape (e.g., assert or guard that identity is SingleKey and then call
toHex()) or define an interface/type with toHex(): string and cast to that type
after the `'toHex' in svcWallet.identity` check; ensure the code uses the
narrowed/typed identity when calling toHex before passing privateKey into
initSvcWorkerWallet so TypeScript no longer requires `as any` and the runtime
guard aligns with the static type (refer to restartWallet, svcWallet.identity,
toHex, SingleKey, and initSvcWorkerWallet).
Summary by CodeRabbit
New Features
Chores
Documentation
Known Issue