feat: enforce min/max sats amount on orders#778
Open
ToRyVand wants to merge 1 commit intolnp2pBot:mainfrom
Open
feat: enforce min/max sats amount on orders#778ToRyVand wants to merge 1 commit intolnp2pBot:mainfrom
ToRyVand wants to merge 1 commit intolnp2pBot:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (10)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughIntroduces a MAX_PAYMENT_AMT environment variable and enforces an upper payment bound across order creation and validation; adds a new i18n message helper and corresponding "less or equal" translations; tests updated to cover max-amount behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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}
Contributor
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
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
ToRyVand
force-pushed
the
feat/issue-406-min-max-sats
branch
from
2129a12 to
4afcbee
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds configurable MAX_PAYMENT_AMT environment variable alongside the existing
MIN_PAYMENT_AMTto enforce sats amount limits on all orders.Closes #406
Changes
.env-sampleMAX_PAYMENT_AMT=10000000bot/validations.tsvalidateSellOrderandvalidateBuyOrderbot/messages.tsmustBeLessEqThanmessage helperbot/modules/orders/scenes.tslocales/*.yaml(10 files)must_be_lt_or_eqkey in all languagestests/bot/validation.spec.tsBehavior
MAX_PAYMENT_AMTis not set, no upper limit is enforcedamount=0) always bypass both min/max checksamount == MAX_PAYMENT_AMTis accepted,amount > MAX_PAYMENT_AMTis rejectedMIN_PAYMENT_AMT(previously only the command-line path did)Tests
141/141 passing — includes new tests:
Summary by CodeRabbit
New Features
Tests