feat: enforce min/max sats amount on orders - #778
Conversation
|
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:
WalkthroughAdds a MAX_PAYMENT_AMT env var and enforces an upper payment bound across handlers and validators; introduces market-order sats estimation, updates wizard input parsing and state, and extends tests and locale messages for "less or equal" validations. ChangesPayment upper-bound and validation flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bot/modules/orders/scenes.ts (1)
307-339:⚠️ Potential issue | 🟡 MinorValidate limits against the normalized sats value.
At Line [329], max validation compares the raw numeric input, but at Line [337] the stored amount is
Math.floor(input). This can reject inputs that would persist as valid integer sats, diverging from command-path behavior.💡 Suggested patch
- const input = Number(ctx.message?.text); + const rawInput = Number(ctx.message?.text); await ctx.deleteMessage(); - if (isNaN(input)) { + if (isNaN(rawInput)) { ctx.wizard.state.error = ctx.i18n.t('not_number'); await ctx.wizard.state.updateUI(); return; } - if (input < 0) { + if (rawInput < 0) { ctx.wizard.state.error = ctx.i18n.t('not_negative'); await ctx.wizard.state.updateUI(); return; } + const input = Math.floor(rawInput); const minPaymentAmt = Number(process.env.MIN_PAYMENT_AMT) || 0; const maxPaymentAmt = Number(process.env.MAX_PAYMENT_AMT) || 0; @@ - ctx.wizard.state.sats = Math.floor(input); + ctx.wizard.state.sats = input;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/modules/orders/scenes.ts` around lines 307 - 339, The validation uses the raw Number(input) for min/max checks but later stores Math.floor(input) into ctx.wizard.state.sats, causing mismatches; normalize the input once (e.g., const normalized = Math.floor(input)) immediately after the NaN/negative checks and use that normalized value for the minPaymentAmt/maxPaymentAmt comparisons and for assigning ctx.wizard.state.sats, while preserving the existing guards that allow 0 as a special case.
🧹 Nitpick comments (2)
.env-sample (1)
54-55: Consider clarifying optional behavior in the sample comment.
To avoid confusion, add a note that leavingMAX_PAYMENT_AMTunset disables the upper bound (as intended for backward compatibility).Based on learnings: document new environment variables in
.env-sampleand keep behavior clear for.envusers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env-sample around lines 54 - 55, Update the comment above the MAX_PAYMENT_AMT env var to explicitly state that this setting is optional and that leaving MAX_PAYMENT_AMT unset disables the upper bound (preserving backward compatibility); mention the units (satoshis) and the default behavior so users know the sample value (10000000) is only an example and not required.tests/bot/validation.spec.ts (1)
146-209: Good boundary coverage for command validations; add wizard-path parity tests too.These additions cover command-based max checks well. Since wizard flow has separate min/max logic, add corresponding tests for
createOrderHandlers.satsto prevent command/wizard drift on edge inputs.Also applies to: 285-342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bot/validation.spec.ts` around lines 146 - 209, Add parallel tests that exercise the wizard flow handler createOrderHandlers.sats mirroring the command-path cases you added for validateSellOrder: stub process.env for MIN_PAYMENT_AMT and MAX_PAYMENT_AMT and test scenarios for amount > MAX (expect failure and reply), amount == MAX (expect success/object), MAX unset (skip max check and succeed), amount 0 with range (market price allowed and result.amount === 0), and amount == MAX+1 (expect failure). Reuse the same sandbox setup and assertion style (replyStub checks, expect(...).to.equal(false) or .to.be.an('object')) so wizard-path logic stays in parity with validateSellOrder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bot/validations.ts`:
- Around line 215-226: Normalize and cache MAX_PAYMENT_AMT as a numeric value
before the max-amount guard in both validateSellOrder and validateBuyOrder:
parse process.env.MAX_PAYMENT_AMT into a const (e.g., maxPaymentAmt =
Number(process.env.MAX_PAYMENT_AMT)), treat non-finite or zero values as
“disabled” (skip the cap), and then only enforce the check when maxPaymentAmt is
a finite > 0 by replacing the existing condition (amount !== 0 &&
process.env.MAX_PAYMENT_AMT && amount > Number(...)) with a condition that uses
maxPaymentAmt (e.g., amount !== 0 && Number.isFinite(maxPaymentAmt) &&
maxPaymentAmt > 0 && amount > maxPaymentAmt) and keep the same
messages.mustBeLessEqThan call using maxPaymentAmt.
---
Outside diff comments:
In `@bot/modules/orders/scenes.ts`:
- Around line 307-339: The validation uses the raw Number(input) for min/max
checks but later stores Math.floor(input) into ctx.wizard.state.sats, causing
mismatches; normalize the input once (e.g., const normalized =
Math.floor(input)) immediately after the NaN/negative checks and use that
normalized value for the minPaymentAmt/maxPaymentAmt comparisons and for
assigning ctx.wizard.state.sats, while preserving the existing guards that allow
0 as a special case.
---
Nitpick comments:
In @.env-sample:
- Around line 54-55: Update the comment above the MAX_PAYMENT_AMT env var to
explicitly state that this setting is optional and that leaving MAX_PAYMENT_AMT
unset disables the upper bound (preserving backward compatibility); mention the
units (satoshis) and the default behavior so users know the sample value
(10000000) is only an example and not required.
In `@tests/bot/validation.spec.ts`:
- Around line 146-209: Add parallel tests that exercise the wizard flow handler
createOrderHandlers.sats mirroring the command-path cases you added for
validateSellOrder: stub process.env for MIN_PAYMENT_AMT and MAX_PAYMENT_AMT and
test scenarios for amount > MAX (expect failure and reply), amount == MAX
(expect success/object), MAX unset (skip max check and succeed), amount 0 with
range (market price allowed and result.amount === 0), and amount == MAX+1
(expect failure). Reuse the same sandbox setup and assertion style (replyStub
checks, expect(...).to.equal(false) or .to.be.an('object')) so wizard-path logic
stays in parity with validateSellOrder.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c284b48-2a37-4002-a663-ac1b39bf3f63
📒 Files selected for processing (15)
.env-samplebot/messages.tsbot/modules/orders/scenes.tsbot/validations.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamltests/bot/validation.spec.ts
2129a12 to
4afcbee
Compare
|
Hey @mostronatorcoder @Catrya @grunch — pinging as suggested by CodeRabbit. I've addressed the review feedback with a new commit: Fix: normalize sats input before min/max validation The wizard was comparing the raw decimal input against Also added 6 wizard-path tests for the min/max bounds (including the decimal normalization case), and exported All 147 tests passing. Thanks for the review! |
|
|
||
| const maxPaymentAmt = Number(process.env.MAX_PAYMENT_AMT); | ||
| if (amount !== 0 && Number.isFinite(maxPaymentAmt) && maxPaymentAmt > 0 && amount > maxPaymentAmt) { | ||
| await messages.mustBeLessEqThan(ctx, 'monto_en_sats', maxPaymentAmt); |
|
Hi @knocte — gentle nudge on this one (open since April). Your note about the Spanish field name is resolved: |
Luquitasjeffrey
left a comment
There was a problem hiding this comment.
Functionally it is broken on orders at market price, this allows me to publish orders for 10-20 ARS or for 20 ARS, which at the time of writing this comment is around 21 sats.
I set the minimum at 100 sats
in orders at market price it should get an estimate amount in sats at the current market price both for fixed amount and range orders
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested
I read the existing discussion and the CodeRabbit comments. Most of the follow-up has been addressed, but one blocking issue remains: market-price orders still bypass the new min/max sats checks because both paths exempt amount === 0. That means tiny fiat orders can still slip through even when their estimated sats value would be below MIN_PAYMENT_AMT or above MAX_PAYMENT_AMT.
Until the estimated sats amount is validated for market-price orders, I can’t approve this PR.
|
|
||
| const maxPaymentAmt = Number(process.env.MAX_PAYMENT_AMT); | ||
| if ( | ||
| amount !== 0 && |
There was a problem hiding this comment.
Blocking: this still exempts amount === 0, so market-price orders bypass the new bounds entirely. If the rule is meant to apply to all orders, validate the estimated sats amount instead of skipping the check for zero-amount orders.
| const input = Math.floor(rawInput); | ||
| const minPaymentAmt = Number(process.env.MIN_PAYMENT_AMT) || 0; | ||
| const maxPaymentAmt = Number(process.env.MAX_PAYMENT_AMT) || 0; | ||
| if (input !== 0 && minPaymentAmt > 0 && input < minPaymentAmt) { |
There was a problem hiding this comment.
Same blocker in the wizard flow: input === 0 skips both min and max checks. That allows market-price orders with tiny fiat values to pass even when the estimated sats amount would violate the configured limits.
|
Thanks @luquitas, you were right. Market-price orders ( Fixed in |
79a56b2 to
7e37d9a
Compare
|
Rebased on |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/bot/validation.spec.ts (1)
147-155: ⚡ Quick winDeduplicate repeated
process.envsandbox setup in tests.The same env sandbox bootstrap is copy-pasted across many cases. A small helper would reduce fixture drift and simplify future bounds-test additions.
Refactor sketch
+const stubEnv = (overrides = {}) => { + sandbox.restore(); + sandbox = sinon.createSandbox(); + sandbox.stub(process, 'env').value({ + MIN_PAYMENT_AMT: 100, + NODE_ENV: 'test', + ...overrides, + }); +}; - sandbox.restore(); - sandbox = sinon.createSandbox(); - sandbox.stub(process, 'env').value({ - MIN_PAYMENT_AMT: 100, - MAX_PAYMENT_AMT: 5000, - NODE_ENV: 'test', - }); + stubEnv({ MAX_PAYMENT_AMT: 5000 });Also applies to: 163-170, 183-190, 199-206, 287-294, 303-309, 318-323, 334-339, 1251-1257, 1267-1272, 1289-1293, 1303-1307, 1317-1321, 1376-1383, 1447-1454, 1503-1510
🤖 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 `@tests/bot/validation.spec.ts` around lines 147 - 155, The test file contains repeated sandbox and process.env stub setup code across multiple test cases. Create a helper function (e.g., setupEnvSandbox) that takes an environment object as a parameter, restores any existing sandbox, creates a new sandbox, and stubs process.env with the provided values. Then replace all instances of the copy-pasted sandbox.restore(), sandbox creation, and env stub pattern throughout the file with calls to this helper function, passing in the required environment variables for each test case.
🤖 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 `@tests/bot/validation.spec.ts`:
- Around line 1496-1531: Add two additional test cases to the validateBuyOrder
(market price sats estimate) describe block in validation.spec.ts to cover the
edge cases that the production logic handles. First, add a test that verifies
rejection when the checkMarketOrderSatsLimits stub resolves with status
'above_max' (similar to the below_min test, expecting false), and second, add a
test that verifies pass-through when the stub resolves with status
'price_unavailable' (similar to the ok test, expecting an object). Both tests
should follow the same pattern as the existing tests: create the stub, call
loadWith(checkStub) to get the validate function, set ctx.state.command.args,
call validate(ctx), and assert the expected result.
---
Nitpick comments:
In `@tests/bot/validation.spec.ts`:
- Around line 147-155: The test file contains repeated sandbox and process.env
stub setup code across multiple test cases. Create a helper function (e.g.,
setupEnvSandbox) that takes an environment object as a parameter, restores any
existing sandbox, creates a new sandbox, and stubs process.env with the provided
values. Then replace all instances of the copy-pasted sandbox.restore(), sandbox
creation, and env stub pattern throughout the file with calls to this helper
function, passing in the required environment variables for each test case.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b06e2fe2-d983-4f37-a94c-fc9847bf2bd2
📒 Files selected for processing (17)
.env-samplebot/messages.tsbot/modules/community/communityContext.tsbot/modules/orders/scenes.tsbot/validations.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamltests/bot/validation.spec.tsutil/index.ts
✅ Files skipped from review due to trivial changes (7)
- locales/fr.yaml
- locales/ko.yaml
- locales/pt.yaml
- locales/es.yaml
- locales/ru.yaml
- locales/fa.yaml
- locales/uk.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- bot/modules/community/communityContext.ts
- locales/de.yaml
- locales/en.yaml
- bot/messages.ts
- .env-sample
- locales/it.yaml
- util/index.ts
- bot/validations.ts
- bot/modules/orders/scenes.ts
Mirrors the sell-side coverage so validateBuyOrder asserts all four checkMarketOrderSatsLimits statuses (below_min, above_max, ok, price_unavailable), per CodeRabbit review on lnp2pBot#778.
|
Thanks @coderabbitai — added the two missing buy-side cases ( |
|
Tip For best results, initiate chat on the files or code changes.
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@Luquitasjeffrey @ermeme — the market-price bypass you both flagged has been fixed since |
|
@Luquitasjeffrey @ermeme — the market-price bypass was fixed in |
There was a problem hiding this comment.
Strict review — current head
I read the complete PR conversation and reviewed current head 39904a898bf2817c02cb03e77bcf51f81ad2fd39.
The previous literal amount === 0 bypass is partially fixed: when the oracle is available, command and wizard publication paths estimate the market-order amount (including ranges and margins) and apply the configured bounds. However, two blocking paths still bypass the guarantee:
- Oracle failures are handled fail-open, so an unbounded market order is still published whenever the estimate cannot be obtained.
- The actual market-price amount is recomputed when the order is taken but is never revalidated. Price movement between publication and take can therefore produce a final amount below
MIN_PAYMENT_AMTor aboveMAX_PAYMENT_AMT.
Please fail closed with user-visible feedback when configured bounds cannot be evaluated, and validate the final post-margin, floored sats amount in both take paths before mutating the order or creating/resolving the invoice. Add regression tests for oracle failure and for a price change between publication and take.
The PR description is also stale: it still says market-price orders always bypass both checks, which contradicts the current implementation and the stated all-orders guarantee.
CI is green. TypeScript, ESLint, Prettier, locale YAML parsing, and git diff --check pass locally. Focused runtime tests could not load locally because the native canvas.node binding is unavailable (pixman-1 is missing); GitHub CI runs the suite successfully.
Verdict: Request changes.
| ); | ||
| return false; | ||
| } | ||
| if (check.status === 'price_unavailable') { |
There was a problem hiding this comment.
🔴 Blocking — bounds fail open on oracle errors. price_unavailable only logs a warning and this validator still returns a valid market order; the buy validator and wizard do the same. A network error, malformed response, unsupported price currency, or missing endpoint therefore recreates the previous bypass and permits tiny/oversized orders while limits are configured. The tests at tests/bot/validation.spec.ts:1486-1493 and 1542-1549 explicitly assert this pass-through. Fail closed with a user-visible error whenever an active bound cannot be evaluated.
| // For market price orders (amount === 0) the final sats are only known when the | ||
| // order is taken. To avoid publishing orders that would settle below/above the | ||
| // configured limits, we estimate the sats at the current market price using the | ||
| // same formula applied at take time (see bot/commands.ts), and validate that |
There was a problem hiding this comment.
🔴 Blocking — a publication-time estimate does not enforce the final amount. Both take paths recompute the market amount later (bot/commands.ts:163-170 and 470-478) and assign it directly to order.amount without checking MIN_PAYMENT_AMT/MAX_PAYMENT_AMT. For example, an order accepted at 100 sats can become 50 sats or 10,000 sats after the rate changes; range orders have the same issue after the taker chooses a fiat amount. Revalidate the exact post-margin, floored amount before mutating the order or creating/resolving an invoice, with tests that change the oracle price between publish and take.
Two gaps in the market-price MIN/MAX enforcement, from the PR lnp2pBot#778 review: 1. Publication failed open: when the price oracle was unavailable the order was published unbounded. It now fails closed with user feedback, but only when bounds are configured (checkMarketOrderSatsLimits short-circuits to 'ok' when neither MIN nor MAX is set, so orders are never needlessly blocked and no price lookup is made). 2. The definitive sats are computed when the order is taken but were never revalidated, so price movement between publication and take could settle an order below MIN or above MAX. Both take paths (addInvoice and showHoldInvoice) now re-check via satsLimitViolation before mutating the order or creating the hold invoice. Tests: oracle-unavailable now asserts fail-closed; add satsLimitViolation unit tests (below_min, above_max, within-bounds, no-bounds, min-only).
|
Thanks for the strict re-review @ermeme — both gaps are addressed in
Regression tests added (oracle-unavailable now asserts fail-closed; One honest note: when a take is aborted for drifting out of bounds, the maker's order stays in its pre-invoice state until the cancel-orders job reclaims it at expiration — same as the existing price-unavailable abort. Happy to add a proactive cancel/notify if you'd prefer that here. |
Add configurable MAX_PAYMENT_AMT environment variable alongside the existing MIN_PAYMENT_AMT to enforce sats amount limits on orders. Changes: - Add MAX_PAYMENT_AMT to .env-sample (default: 10000000) - Add max validation to validateSellOrder and validateBuyOrder - Add min/max validation to the interactive wizard (scenes.ts) - Add mustBeLessEqThan message helper in messages.ts - Add must_be_lt_or_eq locale key in all 10 languages - Add tests for max exceeded, equal to max, boundary, and market price (amount=0) edge cases The max check is optional: if MAX_PAYMENT_AMT is not set, no upper limit is enforced (backwards compatible). Amount 0 (market price orders) always bypasses both min and max checks. Closes lnp2pBot#406
Ensure Math.floor is applied before comparing against MIN/MAX_PAYMENT_AMT so the value checked matches the value stored in wizard state. Also export createOrderHandlers and add wizard-path tests for min/max bounds.
Replace the truthy-string check with Number.isFinite so NaN and Infinity are explicitly handled. Also clarify in .env-sample that MAX_PAYMENT_AMT is optional and leaving it unset disables the upper bound.
Market price orders (amount === 0) bypassed the MIN/MAX_PAYMENT_AMT checks because their sats are only known at take time. Estimate the sats at the current market price (same formula as take time, incl. price margin) and validate the estimate. For ranges, the lower bound is checked against MIN and the upper against MAX. If the price oracle is unavailable the order is allowed through. Covers the command path (validateSellOrder/validateBuyOrder) and the order wizard.
Mirrors the sell-side coverage so validateBuyOrder asserts all four checkMarketOrderSatsLimits statuses (below_min, above_max, ok, price_unavailable), per CodeRabbit review on lnp2pBot#778.
Two gaps in the market-price MIN/MAX enforcement, from the PR lnp2pBot#778 review: 1. Publication failed open: when the price oracle was unavailable the order was published unbounded. It now fails closed with user feedback, but only when bounds are configured (checkMarketOrderSatsLimits short-circuits to 'ok' when neither MIN nor MAX is set, so orders are never needlessly blocked and no price lookup is made). 2. The definitive sats are computed when the order is taken but were never revalidated, so price movement between publication and take could settle an order below MIN or above MAX. Both take paths (addInvoice and showHoldInvoice) now re-check via satsLimitViolation before mutating the order or creating the hold invoice. Tests: oracle-unavailable now asserts fail-closed; add satsLimitViolation unit tests (below_min, above_max, within-bounds, no-bounds, min-only).
16b7257 to
5d0c94d
Compare
|
Rebased on latest |
Summary
Adds configurable MAX_PAYMENT_AMT environment variable alongside the existing
MIN_PAYMENT_AMTto enforce sats amount limits on all orders — including market-price orders, both at publication and at take time.Closes #406
Changes
.env-sampleMAX_PAYMENT_AMT=10000000bot/validations.tsvalidateSellOrder/validateBuyOrder; market-price orders validated against a live-price estimate, failing closed when the oracle is down and bounds are configuredbot/commands.tsaddInvoice,showHoldInvoice) before mutating the order or creating the hold invoicebot/messages.tsmustBeLessEqThan, pluscantVerifySatsLimitsMessageandsatsLimitViolationMessageutil/index.tscheckMarketOrderSatsLimits(publication estimate) +satsLimitViolation(take-time re-check)bot/modules/orders/scenes.tslocales/*.yaml(10 files)must_be_lt_or_eqkey in all languagestests/satsLimitViolationunit testsBehavior
MIN_PAYMENT_AMTnorMAX_PAYMENT_AMTis set, no limit is enforced and no price lookup is made.amount=0): sats are only known at take time, so bounds are enforced twice — against a live-price estimate at publication, and re-checked against the definitive sats when the order is taken (price can drift in between).amount == MAX_PAYMENT_AMTis accepted,amount > MAX_PAYMENT_AMTis rejected.MIN_PAYMENT_AMT(previously only the command-line path did).Known limitation
If a market order drifts out of bounds between publication and take, the take is aborted with a message to the taker; the maker's order stays in its pre-invoice state until the cancel-orders job reclaims it at expiration. This matches the existing price-unavailable abort behavior; a proactive cancel/notify can be added if preferred.
Local checks