feat: add swaps and pay-onchain commands - #25
Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe CLI adds ChangesSwap and on-chain CLI
Sequence Diagram(s)Swap commandssequenceDiagram
participant registerSwapInCommand
participant registerSwapOutCommand
participant client
participant HubAPI
participant output
registerSwapInCommand->>client: POST /api/swaps/in with swapAmountSat
client->>HubAPI: create swap
HubAPI-->>client: swapId
registerSwapInCommand->>client: GET /api/swaps/{swapId}
client->>HubAPI: fetch Swap
HubAPI-->>client: Swap
registerSwapInCommand->>output: print Swap
registerSwapOutCommand->>client: POST /api/swaps/out with swapAmountSat and destination
client->>HubAPI: create swap
HubAPI-->>client: swapId
registerSwapOutCommand->>client: GET /api/swaps/{swapId}
client->>HubAPI: fetch Swap
HubAPI-->>client: Swap
registerSwapOutCommand->>output: print Swap
On-chain paymentsequenceDiagram
participant registerPayOnchainCommand
participant client
participant HubAPI
participant output
registerPayOnchainCommand->>registerPayOnchainCommand: validate amount/all options
registerPayOnchainCommand->>client: POST /api/wallet/redeem-onchain-funds
client->>HubAPI: redeem on-chain funds
HubAPI-->>client: txId
registerPayOnchainCommand->>output: print txId
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
324-326: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the output contract wording.
The README says errors are written to
stderrwith amessagefield, but the CLI helper emits JSON viaconsole.logand wraps failures as{ error: ... }. That mismatch will confuse anyone wiring automation against the docs.♻️ Proposed fix
-All commands output JSON to stdout. Errors are written to stderr as JSON with a `message` field. +All commands output JSON to stdout. Errors are written to stdout as JSON with an `error` field.🤖 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 `@README.md` around lines 324 - 326, The README output contract is inconsistent with the CLI behavior: it says errors go to stderr with a message field, but the CLI helper currently emits JSON through console.log and wraps failures as an error field. Update the wording in the Output section to match the actual behavior of the CLI helper and its failure shape, referencing the CLI helper’s error serialization and output path so the docs align with what users will receive.
🤖 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 `@src/commands/pay-onchain.ts`:
- Around line 36-49: In pay-onchain command handling, the validation in the main
command flow only checks presence and mutual exclusivity of opts.amount and
opts.all, so invalid parseInt results can still reach the API. Add explicit
guards in the pay-onchain command before getClient(program) and the
/api/wallet/redeem-onchain-funds call to reject opts.amount and opts.feeRate
when they are provided but not positive integers, alongside the existing
opts.all checks.
In `@src/commands/swap-in.ts`:
- Around line 11-21: The `swap-in` command’s `--amount` parsing in `handleError`
can produce invalid request payloads because `parseInt` allows `NaN`, zero, and
negative values. Update the `requiredOption`/`.action` flow in `swap-in.ts` to
validate `opts.amount` before calling `client.post` so only finite integers
greater than zero are accepted, and reject invalid input with a clear error
before constructing the `/api/swaps/in` request.
In `@src/commands/swap-out.ts`:
- Around line 23-26: The swap-out request payload is always sending destination
as an empty string when the option is omitted, which breaks optional-field
handling. Update the request construction in swap-out.ts around the post call so
the payload only includes destination when opts.destination is actually
provided, using the swap-out command logic and the client.post<SwapResponse>
call as the place to adjust.
- Around line 11-25: Validate the `--amount` option in the `action` handler for
`swap-out` before calling `client.post` in the `handleError` block: `parseInt`
can still yield `NaN`, zero, or negative values, so add a guard on `opts.amount`
(e.g. check it is a positive integer) and throw the requested error message when
invalid. Use the existing `action(async (opts) => ...)` flow and the
`opts.amount` field to locate the fix.
---
Outside diff comments:
In `@README.md`:
- Around line 324-326: The README output contract is inconsistent with the CLI
behavior: it says errors go to stderr with a message field, but the CLI helper
currently emits JSON through console.log and wraps failures as an error field.
Update the wording in the Output section to match the actual behavior of the CLI
helper and its failure shape, referencing the CLI helper’s error serialization
and output path so the docs align with what users will receive.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7592dd89-3eb8-4466-a35b-37ae610f8521
📒 Files selected for processing (7)
README.mdsrc/commands/lookup-swap.tssrc/commands/pay-onchain.tssrc/commands/swap-in.tssrc/commands/swap-out.tssrc/index.tssrc/types.ts
| if (!opts.all && opts.amount === undefined) { | ||
| throw new Error("specify --amount <sats> or --all"); | ||
| } | ||
| if (opts.all && opts.amount !== undefined) { | ||
| throw new Error("--amount and --all are mutually exclusive"); | ||
| } | ||
| const client = getClient(program); | ||
| const result = await client.post<RedeemOnchainFundsResponse>( | ||
| "/api/wallet/redeem-onchain-funds", | ||
| { | ||
| toAddress: address, | ||
| amountSat: opts.amount, | ||
| feeRate: opts.feeRate, | ||
| sendAll: opts.all, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n src/commands/pay-onchain.ts | head -70Repository: getAlby/hub-cli
Length of output: 2500
Add numeric validation for --amount and --fee-rate.
The options are parsed via parseInt, which returns NaN for invalid inputs. Currently, the code checks only for the presence of --amount and mutual exclusivity with --all, but allows NaN or non-positive values to be sent to the API. Add guards to ensure both are positive integers if specified.
Suggested fix
if (!opts.all && opts.amount === undefined) {
throw new Error("specify --amount <sats> or --all");
}
if (opts.all && opts.amount !== undefined) {
throw new Error("--amount and --all are mutually exclusive");
}
+ if (
+ opts.amount !== undefined &&
+ (Number.isNaN(opts.amount) || opts.amount <= 0)
+ ) {
+ throw new Error("--amount must be a positive integer (sats)");
+ }
+ if (
+ opts.feeRate !== undefined &&
+ (Number.isNaN(opts.feeRate) || opts.feeRate <= 0)
+ ) {
+ throw new Error("--fee-rate must be a positive integer");
+ }
const client = getClient(program);📝 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.
| if (!opts.all && opts.amount === undefined) { | |
| throw new Error("specify --amount <sats> or --all"); | |
| } | |
| if (opts.all && opts.amount !== undefined) { | |
| throw new Error("--amount and --all are mutually exclusive"); | |
| } | |
| const client = getClient(program); | |
| const result = await client.post<RedeemOnchainFundsResponse>( | |
| "/api/wallet/redeem-onchain-funds", | |
| { | |
| toAddress: address, | |
| amountSat: opts.amount, | |
| feeRate: opts.feeRate, | |
| sendAll: opts.all, | |
| if (!opts.all && opts.amount === undefined) { | |
| throw new Error("specify --amount <sats> or --all"); | |
| } | |
| if (opts.all && opts.amount !== undefined) { | |
| throw new Error("--amount and --all are mutually exclusive"); | |
| } | |
| if ( | |
| opts.amount !== undefined && | |
| (Number.isNaN(opts.amount) || opts.amount <= 0) | |
| ) { | |
| throw new Error("--amount must be a positive integer (sats)"); | |
| } | |
| if ( | |
| opts.feeRate !== undefined && | |
| (Number.isNaN(opts.feeRate) || opts.feeRate <= 0) | |
| ) { | |
| throw new Error("--fee-rate must be a positive integer"); | |
| } | |
| const client = getClient(program); | |
| const result = await client.post<RedeemOnchainFundsResponse>( | |
| "/api/wallet/redeem-onchain-funds", | |
| { | |
| toAddress: address, | |
| amountSat: opts.amount, | |
| feeRate: opts.feeRate, | |
| sendAll: opts.all, |
🤖 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/commands/pay-onchain.ts` around lines 36 - 49, In pay-onchain command
handling, the validation in the main command flow only checks presence and
mutual exclusivity of opts.amount and opts.all, so invalid parseInt results can
still reach the API. Add explicit guards in the pay-onchain command before
getClient(program) and the /api/wallet/redeem-onchain-funds call to reject
opts.amount and opts.feeRate when they are provided but not positive integers,
alongside the existing opts.all checks.
| .requiredOption( | ||
| "--amount <sats>", | ||
| "Amount to receive on lightning, in satoshis", | ||
| parseInt, | ||
| ) | ||
| .action(async (opts: { amount: number }) => { | ||
| await handleError(async () => { | ||
| const client = getClient(program); | ||
| const initiated = await client.post<SwapResponse>("/api/swaps/in", { | ||
| swapAmountSat: opts.amount, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Validate --amount to prevent invalid payloads.
parseInt accepts malformed strings (e.g., "abc" results in NaN) which JSON.stringify converts to null, sending an invalid payload to the API. It also allows negative numbers and zero. Add validation to ensure the value is a finite integer and strictly greater than zero before construction of the request.
Suggested fix
.action(async (opts: { amount: number }) => {
await handleError(async () => {
+ if (!Number.isFinite(opts.amount) || !Number.isInteger(opts.amount) || opts.amount <= 0) {
+ throw new Error("--amount must be a positive integer (sats)");
+ }
const client = getClient(program);
const initiated = await client.post<SwapResponse>("/api/swaps/in", {
swapAmountSat: opts.amount,
});📝 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.
| .requiredOption( | |
| "--amount <sats>", | |
| "Amount to receive on lightning, in satoshis", | |
| parseInt, | |
| ) | |
| .action(async (opts: { amount: number }) => { | |
| await handleError(async () => { | |
| const client = getClient(program); | |
| const initiated = await client.post<SwapResponse>("/api/swaps/in", { | |
| swapAmountSat: opts.amount, | |
| }); | |
| .requiredOption( | |
| "--amount <sats>", | |
| "Amount to receive on lightning, in satoshis", | |
| parseInt, | |
| ) | |
| .action(async (opts: { amount: number }) => { | |
| await handleError(async () => { | |
| if (!Number.isFinite(opts.amount) || !Number.isInteger(opts.amount) || opts.amount <= 0) { | |
| throw new Error("--amount must be a positive integer (sats)"); | |
| } | |
| const client = getClient(program); | |
| const initiated = await client.post<SwapResponse>("/api/swaps/in", { | |
| swapAmountSat: opts.amount, | |
| }); |
🤖 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/commands/swap-in.ts` around lines 11 - 21, The `swap-in` command’s
`--amount` parsing in `handleError` can produce invalid request payloads because
`parseInt` allows `NaN`, zero, and negative values. Update the
`requiredOption`/`.action` flow in `swap-in.ts` to validate `opts.amount` before
calling `client.post` so only finite integers greater than zero are accepted,
and reject invalid input with a clear error before constructing the
`/api/swaps/in` request.
| .requiredOption( | ||
| "--amount <sats>", | ||
| "Amount to receive on-chain, in satoshis", | ||
| parseInt, | ||
| ) | ||
| .option( | ||
| "--destination <address>", | ||
| "External on-chain address to receive the funds. Omit to swap into the hub's own on-chain wallet.", | ||
| ) | ||
| .action(async (opts: { amount: number; destination?: string }) => { | ||
| await handleError(async () => { | ||
| const client = getClient(program); | ||
| const initiated = await client.post<SwapResponse>("/api/swaps/out", { | ||
| swapAmountSat: opts.amount, | ||
| destination: opts.destination ?? "", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Validate --amount as a positive integer.
parseInt allows invalid inputs like NaN, 0, or negative numbers, which create malformed payloads.
Add a check before the API call:
if (isNaN(opts.amount) || opts.amount <= 0) throw new Error("Amount must be a positive integer");
🤖 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/commands/swap-out.ts` around lines 11 - 25, Validate the `--amount`
option in the `action` handler for `swap-out` before calling `client.post` in
the `handleError` block: `parseInt` can still yield `NaN`, zero, or negative
values, so add a guard on `opts.amount` (e.g. check it is a positive integer)
and throw the requested error message when invalid. Use the existing
`action(async (opts) => ...)` flow and the `opts.amount` field to locate the
fix.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation