fix: change trade limits from fiat to sats#10
Open
Mostrica wants to merge 5 commits intoMostroP2P:mainfrom
Open
fix: change trade limits from fiat to sats#10Mostrica wants to merge 5 commits intoMostroP2P:mainfrom
Mostrica wants to merge 5 commits intoMostroP2P:mainfrom
Conversation
Trade limits are now specified in satoshis instead of fiat units. This ensures consistent risk management across all currencies. Previously, max_trade_amount_fiat: 100 would block a 200 CUP order (~$1.60 USD) while allowing a 100 USD order (~6250x more value). Changes: - config: max_trade_amount_sats, max_daily_volume_sats - safety.ts: checkLimits() and recordTrade() now use sats - create-order.ts: estimates sats from fiat via price API - auto-trade.ts: updated all strategies to use sats - SKILL.md: updated documentation Closes MostroP2P#9
|
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:
WalkthroughTrade limits converted from fiat to satoshis: config keys, types, defaults, and validation now use sats; safety APIs and signatures accept/record sats; trading scripts estimate sats via BTC price (with fallback) for limit checks and recording. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Script as Trading Script
participant Price as BTC Price Service
participant Safety as Safety Module
participant Exchange as Exchange / OrderService
participant Ledger as Trade Recorder
Script->>Price: fetchBtcPrice(currency, api)
Price-->>Script: price (or null)
Script->>Script: estimatedSats = fiat_amount / price (or fallback)
Script->>Safety: checkLimits(limits, estimatedSats)
Safety-->>Script: allowed / rejected
alt allowed
Script->>Exchange: createOrder(...)
Exchange-->>Script: order (with order.amount sats)
Script->>Ledger: recordTrade(order.amount || estimatedSats)
Ledger-->>Script: ack
else rejected
Safety-->>Script: reason
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 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: 6
🧹 Nitpick comments (2)
lib/config.ts (1)
61-68: Silent fallback when users have old fiat-only config.When
loadConfigmerges user config, if the user only has the oldmax_trade_amount_fiat/max_daily_volume_fiatfields, the new sats fields silently fall back to defaults (50000/500000). This could surprise users who had intentionally tightened or loosened their limits. Consider logging a deprecation warning when the old fields are present and the new ones are absent.💡 Example warning
try { const raw = readFileSync(path, "utf-8"); const userConfig = JSON.parse(raw); - return { ...DEFAULT_CONFIG, ...userConfig, limits: { ...DEFAULT_CONFIG.limits, ...userConfig.limits } }; + const merged = { ...DEFAULT_CONFIG, ...userConfig, limits: { ...DEFAULT_CONFIG.limits, ...userConfig.limits } }; + if (userConfig.limits && ('max_trade_amount_fiat' in userConfig.limits || 'max_daily_volume_fiat' in userConfig.limits) && + !('max_trade_amount_sats' in userConfig.limits)) { + console.warn("⚠️ Deprecated: max_trade_amount_fiat/max_daily_volume_fiat are ignored. Migrate to max_trade_amount_sats/max_daily_volume_sats in config.json."); + } + return merged; } catch (e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/config.ts` around lines 61 - 68, The loadConfig merge currently silently falls back to DEFAULT_CONFIG when users have only the old fiat-only fields; update loadConfig (the code that reads userConfig and merges into DEFAULT_CONFIG/limits) to detect if userConfig.limits contains max_trade_amount_fiat or max_daily_volume_fiat while missing the corresponding max_trade_amount_sats or max_daily_volume_sats, and emit a clear deprecation/warning log (console.warn or logger.warn) mentioning the old keys are deprecated and advising the user to add the new sats keys; keep the existing merge behavior but do not overwrite or change values automatically—just log the warning so users are notified.scripts/create-order.ts (1)
64-74: Hardcoded fallback rate will drift; consider making it configurable or failing safe.The
opts.fiat_amount * 1000fallback assumes $100k/BTC. If BTC appreciates significantly, the fallback underestimates the sats value, making limits more permissive than intended — which is the wrong direction for a safety check. Consider either:
- Blocking the trade when price cannot be fetched (safer), or
- Making the fallback rate configurable.
This same pattern is duplicated in
scripts/auto-trade.ts(Lines 121-123 and 323-325). If you keep the fallback, extract a shared helper to avoid drift between the two files.♻️ Example: extract a shared helper
In
lib/safety.ts:+/** + * Estimate sats from a fiat amount using a BTC price, with a conservative fallback. + * Falls back to 2000 sats/USD equivalent to err on the restrictive side. + */ +export async function estimateSatsFromFiat( + fiatAmount: number, + fiatCode: string, + apiUrl?: string +): Promise<{ sats: number; estimated: boolean }> { + const btcPrice = await fetchBtcPrice(fiatCode, apiUrl); + if (btcPrice && btcPrice > 0) { + return { sats: Math.round((fiatAmount / btcPrice) * 1e8), estimated: false }; + } + // Conservative fallback — overestimates sats to keep limits restrictive + return { sats: fiatAmount * 2000, estimated: true }; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/create-order.ts` around lines 64 - 74, The hardcoded fallback (estimatedSats = opts.fiat_amount * 1000) is unsafe and duplicated; replace it by either failing-safe (throw or reject the order when fetchBtcPrice returns null) or by using a configurable fallback rate, and extract the logic into a shared helper (e.g., getEstimatedSats(btcPrice, fiatAmount, fallbackSatsPerUsd or fallbackBtcPrice)) so both places reuse the same behavior; update code that currently calls fetchBtcPrice and computes estimatedSats to call this helper (refer to symbols fetchBtcPrice, estimatedSats, opts.fiat_amount and the new getEstimatedSats helper) and read the fallback from configuration (e.g., config.fallback_btc_price or config.fallback_sats_per_usd) rather than using the hardcoded 1000.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/config.ts`:
- Around line 82-84: Add the same positive-value validation for
max_daily_volume_sats as exists for max_trade_amount_sats: in the config
validation function where config.limits is checked (the block that currently
checks config.limits.max_trade_amount_sats and pushes to errors), add a check
for config.limits.max_daily_volume_sats <= 0 and push a descriptive error like
"max_daily_volume_sats must be positive" to the errors array; ensure you
reference the same config and errors variables so behavior matches the existing
validation.
In `@lib/safety.ts`:
- Around line 140-144: The existing daily_volume entries in trade-state.json may
be fiat from the old release and will be mixed with sats in recordTrade
(function: recordTrade, uses loadState and todayKey), so update the
state-loading/migration logic to detect and reset stale fiat values: in
loadState (or a one-time migration run before recordTrade), scan
state.daily_volume entries and if a value is clearly not sats (e.g.,
non-integer, negative, or below a sensible minimum threshold like <1000 sats)
replace it with 0 (or remove the key) and persist the cleaned state; optionally
add a migration flag/version field to avoid re-checking every run.
In `@scripts/auto-trade.ts`:
- Around line 175-176: The line computing actualSats uses nullish coalescing
with p?.order?.amount which can be 0 for market-price DCA orders, so it
incorrectly keeps 0 instead of falling back to estimatedSats; change the
fallback logic to use a falsy check (use || or an explicit check) so actualSats
= p?.order?.amount || estimatedSats, then pass that value to recordTrade; update
the expression around actualSats, p?.order?.amount, estimatedSats and ensure
recordTrade receives the corrected value.
- Around line 248-251: The code currently uses best.amount directly (satsAmount
= best.amount) which can be 0 for range orders and bypass
checkLimits/recordTrade; update the logic in the block around
fiatAmount/satsAmount to detect when best.amount is 0 and derive a fallback sats
estimate from the fiatAmount (like the DCA/create-order pattern does—convert
fiat to sats using the matched price or current spot rate and round to integer
sats) before calling checkLimits and recordTrade; ensure the fallback only
applies when best.amount === 0 and keep variable name satsAmount and functions
checkLimits and recordTrade unchanged so downstream code still uses the
estimated sats.
- Around line 321-327: The current call to checkLimits(config.limits,
estimatedSats * 2) treats both buy and sell as one trade and can incorrectly
trigger the per-trade cap; instead call checkLimits separately for each side
using estimatedSats (e.g., validate firstOrderLimit = checkLimits(config.limits,
estimatedSats) and secondOrderLimit = checkLimits(config.limits,
estimatedSats)), and also perform a daily-volume check against config.limits
(e.g., ensure todayVolume + estimatedSats * 2 <= max_daily_volume_sats) before
proceeding; reference symbols: estimatedSats, checkLimits, config.limits,
max_trade_amount_sats, todayVolume, and recordTrade when updating volume.
In `@scripts/create-order.ts`:
- Around line 176-178: The code uses the nullish coalescing operator when
computing actualSats which lets a zero amount from the exchange through and
incorrectly records 0; change the fallback logic in scripts/create-order.ts
where actualSats is computed (variable actualSats and the call
recordTrade(actualSats)) to use the logical OR (||) so falsy zero falls back to
estimatedSats, and apply the same change in auto-trade.ts at the corresponding
computation (the equivalent actualSats/amount handling) to ensure market-price
orders with amount: 0 use the estimatedSats before calling recordTrade.
---
Nitpick comments:
In `@lib/config.ts`:
- Around line 61-68: The loadConfig merge currently silently falls back to
DEFAULT_CONFIG when users have only the old fiat-only fields; update loadConfig
(the code that reads userConfig and merges into DEFAULT_CONFIG/limits) to detect
if userConfig.limits contains max_trade_amount_fiat or max_daily_volume_fiat
while missing the corresponding max_trade_amount_sats or max_daily_volume_sats,
and emit a clear deprecation/warning log (console.warn or logger.warn)
mentioning the old keys are deprecated and advising the user to add the new sats
keys; keep the existing merge behavior but do not overwrite or change values
automatically—just log the warning so users are notified.
In `@scripts/create-order.ts`:
- Around line 64-74: The hardcoded fallback (estimatedSats = opts.fiat_amount *
1000) is unsafe and duplicated; replace it by either failing-safe (throw or
reject the order when fetchBtcPrice returns null) or by using a configurable
fallback rate, and extract the logic into a shared helper (e.g.,
getEstimatedSats(btcPrice, fiatAmount, fallbackSatsPerUsd or fallbackBtcPrice))
so both places reuse the same behavior; update code that currently calls
fetchBtcPrice and computes estimatedSats to call this helper (refer to symbols
fetchBtcPrice, estimatedSats, opts.fiat_amount and the new getEstimatedSats
helper) and read the fallback from configuration (e.g.,
config.fallback_btc_price or config.fallback_sats_per_usd) rather than using the
hardcoded 1000.
Comment on lines
+140
to
+144
| export function recordTrade(satsAmount: number): void { | ||
| const state = loadState(); | ||
| const today = todayKey(); | ||
|
|
||
| state.daily_volume[today] = (state.daily_volume[today] ?? 0) + fiatAmount; | ||
| state.daily_volume[today] = (state.daily_volume[today] ?? 0) + satsAmount; |
There was a problem hiding this comment.
Stale fiat values in trade-state.json after upgrade.
Existing daily_volume entries in ~/.mostro-skill/trade-state.json contain fiat amounts from the old code. After upgrading, new sats values will be summed with leftover fiat values. Since fiat numbers are orders of magnitude smaller than sats, this won't cause false blocks — and old keys auto-purge after 7 days — but it is semantically incorrect during that window. Consider either documenting this in a migration note or resetting stale entries on first run.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/safety.ts` around lines 140 - 144, The existing daily_volume entries in
trade-state.json may be fiat from the old release and will be mixed with sats in
recordTrade (function: recordTrade, uses loadState and todayKey), so update the
state-loading/migration logic to detect and reset stale fiat values: in
loadState (or a one-time migration run before recordTrade), scan
state.daily_volume entries and if a value is clearly not sats (e.g.,
non-integer, negative, or below a sensible minimum threshold like <1000 sats)
replace it with 0 (or remove the key) and persist the cleaned state; optionally
add a migration flag/version field to avoid re-checking every run.
Author
There was a problem hiding this comment.
Intentionally not addressed. As you noted, fiat values are orders of magnitude smaller than sats (~$100 fiat → 100 "contaminated sats" vs 50,000+ real sats), so they cannot cause false blocks. The 7-day auto-purge handles cleanup naturally.
Adding migration logic would be over-engineering for a self-resolving edge case that affects at most one week of early adopters.
There was a problem hiding this comment.
@Mostrica, understood — your reasoning is solid. The magnitude difference makes the risk negligible, and the auto-purge provides a clean resolution. No need to over-engineer a self-correcting edge case. 👍
scripts/create-order.ts
Outdated
Comment on lines
176
to
178
| // Use actual sats from order response, or estimate | ||
| const actualSats = order?.amount ?? estimatedSats; | ||
| recordTrade(actualSats); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the code at lines 176-178 in create-order.ts
head -n 200 scripts/create-order.ts | tail -n 30Repository: MostroP2P/mostro-skill
Length of output: 1295
🏁 Script executed:
# Get more context around the problematic lines
sed -n '160,190p' scripts/create-order.tsRepository: MostroP2P/mostro-skill
Length of output: 1250
🏁 Script executed:
# Check if the same pattern exists in auto-trade.ts (as suggested in the request)
rg -n 'order\?\.\s*amount\s*\?\?' --type=tsRepository: MostroP2P/mostro-skill
Length of output: 240
🏁 Script executed:
# Search for the recordTrade function definition to understand its expectations
rg -A 5 'function recordTrade|const recordTrade' --type=tsRepository: MostroP2P/mostro-skill
Length of output: 408
🏁 Script executed:
# Look for the order type definition or interface
rg -B 5 -A 10 'interface.*Order|type.*Order' --type=ts | head -n 100Repository: MostroP2P/mostro-skill
Length of output: 4454
Use || instead of ?? to properly handle zero-amount orders.
When Mostro returns amount: 0 for market-price orders, the nullish coalescing operator (??) passes 0 directly to recordTrade() since it only checks for null/undefined, not falsy values. This records 0 sats for the trade, breaking daily volume tracking.
The same issue exists in auto-trade.ts at line 175.
🐛 Proposed fixes
- const actualSats = order?.amount ?? estimatedSats;
+ const actualSats = order?.amount || estimatedSats;- const actualSats = p?.order?.amount ?? estimatedSats;
+ const actualSats = p?.order?.amount || estimatedSats;📝 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.
Suggested change
| // Use actual sats from order response, or estimate | |
| const actualSats = order?.amount ?? estimatedSats; | |
| recordTrade(actualSats); | |
| // Use actual sats from order response, or estimate | |
| const actualSats = order?.amount || estimatedSats; | |
| recordTrade(actualSats); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/create-order.ts` around lines 176 - 178, The code uses the nullish
coalescing operator when computing actualSats which lets a zero amount from the
exchange through and incorrectly records 0; change the fallback logic in
scripts/create-order.ts where actualSats is computed (variable actualSats and
the call recordTrade(actualSats)) to use the logical OR (||) so falsy zero falls
back to estimatedSats, and apply the same change in auto-trade.ts at the
corresponding computation (the equivalent actualSats/amount handling) to ensure
market-price orders with amount: 0 use the estimatedSats before calling
recordTrade.
added 2 commits
February 18, 2026 22:08
Users can adjust limits in their local config.json as needed.
- Add validation for max_daily_volume_sats in config - Use || instead of ?? for market-price orders (amount=0) - Handle best.amount=0 for range orders in limit strategy - Fix market maker limit check: verify per-trade individually, daily volume combined - Export loadState and todayKey for proper limit checking
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
lib/safety.ts (3)
167-178:fetchhas no timeout — a hanging API will block the script indefinitely.If
yadio.iois slow or unresponsive, every strategy execution stalls. Consider adding anAbortSignal.timeout:Suggested fix
- const res = await fetch(apiUrl); + const res = await fetch(apiUrl, { signal: AbortSignal.timeout(10_000) });
AbortSignal.timeoutis available in Node 18+. The abort triggers thecatchpath, which already returnsnull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/safety.ts` around lines 167 - 178, The fetch in fetchBtcPrice can hang; create an AbortSignal via AbortSignal.timeout (Node 18+) and pass it to fetch to enforce a timeout (e.g., 3–5s), e.g. const signal = AbortSignal.timeout(5000) then await fetch(apiUrl, { signal }); keep the existing try/catch so the abort throws into the catch and returns null; reference the fetchBtcPrice function and apiUrl parameter when making this change.
140-144: No input validation onsatsAmount.
recordTradeunconditionally addssatsAmountto the volume. If a caller passes0,NaN, or a negative number (e.g., from a failed parse), the daily volume tracking silently corrupts. A guard at the top would be cheap insurance:Suggested guard
export function recordTrade(satsAmount: number): void { + if (!satsAmount || satsAmount <= 0 || !Number.isFinite(satsAmount)) { + console.warn(`⚠️ recordTrade called with invalid amount: ${satsAmount}`); + return; + } const state = loadState();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/safety.ts` around lines 140 - 144, The recordTrade function currently trusts satsAmount and may corrupt state.daily_volume if passed 0, NaN, negative or non-finite values; add an input guard at the top of recordTrade that validates satsAmount is a finite positive number (and optionally an integer), returning early (or throwing) if the value is invalid; use the existing symbols recordTrade, loadState, todayKey and state.daily_volume to locate the code and ensure only valid satsAmount values are added to state.daily_volume[today].
57-73: Consider encapsulating state access instead of exporting internals.
loadState()andtodayKey()are exported solely soauto-trade.tscan manually readdaily_volumefor the market-maker pre-check. This couples callers to the internalTradeStateshape. A thin helper would be cleaner:Suggested helper
/** Get today's accumulated trade volume in sats */ export function getTodayVolume(): number { const state = loadState(); return state.daily_volume[todayKey()] ?? 0; }Then
auto-trade.tscallsgetTodayVolume()without importingloadState/todayKey.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/safety.ts` around lines 57 - 73, Add a small exported accessor function named getTodayVolume() that calls loadState() and todayKey() and returns state.daily_volume[todayKey()] ?? 0 so callers (like auto-trade.ts) no longer import internal shapes; export getTodayVolume and update auto-trade.ts to call getTodayVolume() instead of loadState()/todayKey(), and leave loadState/todayKey as internals (or stop using them externally) to decouple callers from the TradeState shape.scripts/create-order.ts (1)
70-74: Fallback assumes USD-like currency strength.The
fiat_amount * 1000heuristic maps 1 fiat unit ≈ 1000 sats, which holds only near USD at ~$100k BTC. For weak currencies (e.g., ARS, VES, NGN), this massively overestimates sats and will spuriously block trades when the price API is down. Since the whole PR motivation is consistent behavior across currencies, consider either rejecting the trade outright when the price is unavailable, or using a more explicit guard:Suggested approach
} else { - // If we can't get price, use a conservative estimate (assume $100k BTC) - console.warn("⚠️ Could not fetch BTC price, using conservative estimate for limit check"); - estimatedSats = opts.fiat_amount * 1000; // ~1000 sats per dollar at $100k + // Cannot verify sats amount without a price — reject to avoid inconsistent limits + console.error("❌ Could not fetch BTC price — cannot perform sats-based limit check"); + auditLog({ + timestamp: new Date().toISOString(), + action: "create-order", + fiat_amount: opts.fiat_amount, + fiat_code: opts.currency, + result: "rejected", + details: "BTC price unavailable", + }); + process.exit(1); }The same pattern exists in
scripts/auto-trade.ts(lines 121-123, 331-333, and 255-257). If you decide to keep the fallback, at minimum update the comment to clarify it's USD-centric.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/create-order.ts` around lines 70 - 74, The fallback that sets estimatedSats = opts.fiat_amount * 1000 is USD-biased and can falsely block trades; update create-order.ts to instead fail fast when the BTC price fetch fails (throw or return an error/HTTP 503) so we don’t guess across currencies, or if you must keep a fallback, scope it explicitly to USD by checking opts.fiat_currency === 'USD' before applying the 1000 sats heuristic and otherwise reject; apply the same change to the matching fallbacks in scripts/auto-trade.ts (the occurrences around lines ~121-123, ~255-257, ~331-333), and update the comment near estimatedSats to clearly state the USD-only assumption if you retain the heuristic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/auto-trade.ts`:
- Around line 394-395: The two recordTrade(estimatedSats) calls are executed
immediately after sendGiftWrap, inflating daily volume if orders fail; update
the flow in scripts/auto-trade.ts so you wait for and verify Mostro's responses
from sendGiftWrap (iterate the returned response(s) like the DCA strategy does)
and only call recordTrade(estimatedSats) after confirming success for each
corresponding order, or alternatively add explicit error-handling/documentation
if you intentionally accept the trade-risk; reference sendGiftWrap and
recordTrade to locate and modify the post-order logic.
- Around line 329-348: The market-maker path currently enforces per-trade and
daily-volume checks locally (using estimatedSats, fetchBtcPrice, loadState,
todayKey, and config.limits.max_trade_amount_sats) but skips checkLimits which
enforces max_trades_per_day and cooldown_seconds; update the flow to call
checkLimits(...) for the first order (so max_trades_per_day and cooldown_seconds
are validated using existing logic) and then perform the manual daily-volume
check for the combined two-order amount (todayVolume + estimatedSats * 2 against
config.limits.max_daily_volume_sats) before proceeding with the second/order
pair, ensuring you still use estimatedSats and preserve the existing fallback
behavior when fetchBtcPrice is unavailable.
---
Duplicate comments:
In `@scripts/auto-trade.ts`:
- Around line 175-177: No change required: the logic using the short-circuit
operator correctly handles market-price orders where p?.order?.amount may be 0
by falling back to estimatedSats; keep the line const actualSats =
p?.order?.amount || estimatedSats; and the subsequent recordTrade(actualSats);
as-is (no code edits needed to p?.order?.amount, estimatedSats, or recordTrade).
- Around line 251-259: The change estimates satsAmount when best.amount is falsy
by calling fetchBtcPrice(strategy.currency, config.price_api) and falling back
to Math.round(fiatAmount * 1000), but ensure the code validates the computed
satsAmount is a positive integer before calling checkLimits(config.limits,
satsAmount): after computing satsAmount (the variable), if satsAmount <= 0 then
log/throw a clear error or set a safe minimum and only then call checkLimits;
reference best.amount, fetchBtcPrice, strategy.currency, config.price_api,
satsAmount, and checkLimits in your fix.
In `@scripts/create-order.ts`:
- Around line 176-178: The current assignment should use the logical OR fallback
so market-price orders with amount=0 fall back to estimatedSats: keep or change
to "const actualSats = order?.amount || estimatedSats" (the variable actualSats
in scripts/create-order.ts) and ensure recordTrade(actualSats) is called; also
search for other uses of nullish coalescing (??) for order.amount and replace
them with || where a value of 0 must fall back to the estimate.
---
Nitpick comments:
In `@lib/safety.ts`:
- Around line 167-178: The fetch in fetchBtcPrice can hang; create an
AbortSignal via AbortSignal.timeout (Node 18+) and pass it to fetch to enforce a
timeout (e.g., 3–5s), e.g. const signal = AbortSignal.timeout(5000) then await
fetch(apiUrl, { signal }); keep the existing try/catch so the abort throws into
the catch and returns null; reference the fetchBtcPrice function and apiUrl
parameter when making this change.
- Around line 140-144: The recordTrade function currently trusts satsAmount and
may corrupt state.daily_volume if passed 0, NaN, negative or non-finite values;
add an input guard at the top of recordTrade that validates satsAmount is a
finite positive number (and optionally an integer), returning early (or
throwing) if the value is invalid; use the existing symbols recordTrade,
loadState, todayKey and state.daily_volume to locate the code and ensure only
valid satsAmount values are added to state.daily_volume[today].
- Around line 57-73: Add a small exported accessor function named
getTodayVolume() that calls loadState() and todayKey() and returns
state.daily_volume[todayKey()] ?? 0 so callers (like auto-trade.ts) no longer
import internal shapes; export getTodayVolume and update auto-trade.ts to call
getTodayVolume() instead of loadState()/todayKey(), and leave loadState/todayKey
as internals (or stop using them externally) to decouple callers from the
TradeState shape.
In `@scripts/create-order.ts`:
- Around line 70-74: The fallback that sets estimatedSats = opts.fiat_amount *
1000 is USD-biased and can falsely block trades; update create-order.ts to
instead fail fast when the BTC price fetch fails (throw or return an error/HTTP
503) so we don’t guess across currencies, or if you must keep a fallback, scope
it explicitly to USD by checking opts.fiat_currency === 'USD' before applying
the 1000 sats heuristic and otherwise reject; apply the same change to the
matching fallbacks in scripts/auto-trade.ts (the occurrences around lines
~121-123, ~255-257, ~331-333), and update the comment near estimatedSats to
clearly state the USD-only assumption if you retain the heuristic.
Comment on lines
+394
to
+395
| recordTrade(estimatedSats); | ||
| recordTrade(estimatedSats); |
There was a problem hiding this comment.
Trades recorded before confirmation.
Both recordTrade calls execute immediately after sendGiftWrap without waiting for Mostro to confirm the orders succeeded. If either order is rejected, the daily volume is inflated, potentially blocking subsequent legitimate trades. The DCA strategy at least iterates responses before recording. Consider deferring recording until responses are verified, or accepting this as a known trade-off and documenting it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/auto-trade.ts` around lines 394 - 395, The two
recordTrade(estimatedSats) calls are executed immediately after sendGiftWrap,
inflating daily volume if orders fail; update the flow in scripts/auto-trade.ts
so you wait for and verify Mostro's responses from sendGiftWrap (iterate the
returned response(s) like the DCA strategy does) and only call
recordTrade(estimatedSats) after confirming success for each corresponding
order, or alternatively add explicit error-handling/documentation if you
intentionally accept the trade-risk; reference sendGiftWrap and recordTrade to
locate and modify the post-order logic.
Changed checkLimits call from fiat_amount*2 to estimatedSats*2, matching the original behavior but with sats instead of fiat.
Mostrica
force-pushed
the
fix/limits-in-sats
branch
from
ef00c9a to
0cf39db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/auto-trade.ts`:
- Around line 119-123: The fallback path that computes estimatedSats can produce
non-integer values; update the fallback expressions in scripts/auto-trade.ts so
they use Math.round(...) like the limit-order case: replace occurrences that use
strategy.fiat_amount * 1000 with Math.round(strategy.fiat_amount * 1000) (the
computed value used to set estimatedSats before calling recordTrade and
aggregating daily_volume), and apply the same change for the DCA and
market-maker fallback sites (the other occurrences around the same logic) to
ensure sats are integers.
---
Duplicate comments:
In `@scripts/auto-trade.ts`:
- Line 298: recordTrade is being called immediately after sendGiftWrap in the
limit and market-maker strategies (and at the locations around
recordTrade(satsAmount)), which inflates counts if Mostro rejects or never
processes the order; change both strategies to await the Mostro order response
(the promise returned by sendGiftWrap or the order submission function) and only
call recordTrade(satsAmount) after confirming a successful/acknowledged
response, and ensure rejections/timeouts do not call recordTrade and instead
log/handle the failure similarly to the DCA path.
- Around line 335-340: The current call checkLimits(config.limits, estimatedSats
* 2) incorrectly applies the per-trade cap (max_trade_amount_sats) to the
combined buy+sell amount; change this to perform two checks: run a per-trade
check with checkLimits(config.limits, estimatedSats) to ensure each order does
not exceed max_trade_amount_sats, and then run a separate daily/volume check
with checkLimits(config.limits, estimatedSats * 2) (or a dedicated daily-volume
check) and block only if either check returns allowed === false; reference
checkLimits, estimatedSats and max_trade_amount_sats when updating the logic so
the per-trade cap is enforced per-order and the combined amount is used only for
daily/volume limits.
scripts/auto-trade.ts
Outdated
Comment on lines
119
to
123
| // Estimate sats for limit checking | ||
| const btcPrice = await fetchBtcPrice(strategy.currency, config.price_api); | ||
| const estimatedSats = btcPrice && btcPrice > 0 | ||
| ? Math.round((strategy.fiat_amount / btcPrice) * 1e8) | ||
| : strategy.fiat_amount * 1000; // fallback |
There was a problem hiding this comment.
Missing Math.round() in the fallback path for DCA and market-maker strategies.
The limit-order strategy (Line 257) correctly wraps the fallback with Math.round(fiatAmount * 1000), but the DCA and market-maker fallbacks omit it. If fiat_amount is a non-integer (e.g. 0.1), floating-point arithmetic yields 100.00000000000001 instead of 100, which then propagates into recordTrade and inflates the stored daily_volume with a fractional sats value.
🐛 Proposed fix
- : strategy.fiat_amount * 1000; // fallback (line 123 – DCA)
+ : Math.round(strategy.fiat_amount * 1000); // fallback
- : strategy.fiat_amount * 1000; // fallback (line 333 – market-maker)
+ : Math.round(strategy.fiat_amount * 1000); // fallbackAlso applies to: 330-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/auto-trade.ts` around lines 119 - 123, The fallback path that
computes estimatedSats can produce non-integer values; update the fallback
expressions in scripts/auto-trade.ts so they use Math.round(...) like the
limit-order case: replace occurrences that use strategy.fiat_amount * 1000 with
Math.round(strategy.fiat_amount * 1000) (the computed value used to set
estimatedSats before calling recordTrade and aggregating daily_volume), and
apply the same change for the DCA and market-maker fallback sites (the other
occurrences around the same logic) to ensure sats are integers.
DCA and market-maker strategies were missing Math.round() in the fallback path, which could produce non-integer sats values when fiat_amount is a decimal (e.g., 0.1 → 100.00000000000001). This aligns with the limit-order strategy pattern (line 257).
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
Trade limits are now specified in satoshis instead of fiat units. This ensures consistent risk management across all currencies.
Problem
Previously,
max_trade_amount_fiat: 100would block a 200 CUP order (~$1.60 USD) while allowing a 100 USD order (~6250x more value). This made the safety limits ineffective for non-USD currencies.Changes
max_trade_amount_sats,max_daily_volume_sats(replaces_fiatversions)checkLimits()andrecordTrade()now use satsMigration
Users should update their
config.json:"limits": { - "max_trade_amount_fiat": 100, - "max_daily_volume_fiat": 500, + "max_trade_amount_sats": 50000, + "max_daily_volume_sats": 500000, ... }Testing
Closes #9
Summary by CodeRabbit
New Features
Chores