Add counterparty requirements#853
Conversation
WalkthroughThis PR adds counterparty requirement fields to the user model, enforces them during order taking, exposes settings commands to manage them, and updates localization files, tests, and sample environment variables for the new flow. ChangesCounterparty Requirements Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 11
🧹 Nitpick comments (3)
bot/modules/orders/takeOrder.ts (1)
161-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for
meetsCounterpartyRequirements.This is a new enforcement gate affecting both buy/sell take flows, but no tests are included covering the age/order-count thresholds, the 0-disables-check behavior, or the missing-
counterparty_requirementsfallback.🤖 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 `@bot/modules/orders/takeOrder.ts` around lines 161 - 188, Add unit tests for meetsCounterpartyRequirements in takeOrder.ts to cover the new gate behavior in both buy/sell take flows. Verify the min_days_using_bot and min_completed_orders thresholds block and allow correctly, confirm 0 disables each check, and ensure the function returns true when orderCreator.counterparty_requirements is missing. Use mocks for getUserAge and messages.notMeetingRequirementsMessage to assert the right branch behavior.bot/modules/user/scenes/settings.ts (2)
154-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocal variable naming: use camelCase instead of snake_case.
min_days/min_ordersare local variables, not schema fields; per repo guidelines TS variables should use camelCase (e.g.minDays,minOrders).As per coding guidelines,
**/*.{ts,tsx}: "Use camelCase for functions and variables."Also applies to: 202-204
🤖 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 `@bot/modules/user/scenes/settings.ts` around lines 154 - 155, The local variables in the settings scene are using snake_case instead of the repo’s required camelCase style. Update the variable names in the parsing/validation logic in the settings flow, including the `min_days` and `min_orders` values around the `parseInt` checks in `settings.ts`, to use camelCase such as `minDays` and `minOrders`, and make sure any subsequent references in the same methods are renamed consistently.Source: Coding guidelines
144-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate logic between
counterpartyageandcounterpartyordershandlers.Both handlers share nearly identical parse/validate/cap/save/feedback structure, differing only in the target field, i18n keys, and env var. Extracting a shared factory would reduce future divergence risk (e.g. if validation logic needs to change, it'd need updating in two places).
♻️ Proposed refactor sketch
const createRequirementCommand = ( command: string, field: 'min_days_using_bot' | 'min_completed_orders', envVar: string, defaultMax: number, updatedI18nKey: string, paramName: string, ) => async (ctx: CommunityContext) => { try { await ctx.deleteMessage(); const state = ctx.scene.state as CommunityWizardState; if (ctx.message === undefined || !('text' in ctx.message)) throw new Error('ctx.message is undefined'); const [, raw] = ctx.message.text.trim().split(' '); const value = parseInt(raw, 10); if (isNaN(value) || value < 0) throw new Error('NotValidNumber'); const max = readNonNegativeInt(process.env[envVar], defaultMax); if (value > max) { state.error = { i18n: 'invalid_range', command, max }; return await updateMessage(ctx); } const user = state.user; if (!user.counterparty_requirements) { user.counterparty_requirements = { min_days_using_bot: 0, min_completed_orders: 0, }; } user.counterparty_requirements[field] = value; await user.save(); state.feedback = { i18n: updatedI18nKey, [paramName]: value }; await updateMessage(ctx); } catch (err) { logger.error(err); (ctx.scene.state as CommunityWizardState).error = { i18n: err instanceof Error && err.message === 'NotValidNumber' ? 'invalid_number' : 'generic_error', }; await updateMessage(ctx); } }; scene.command('counterpartyage', resetMessage, createRequirementCommand( '/counterpartyage', 'min_days_using_bot', 'MAX_COUNTERPARTY_AGE_REQUIREMENT', 30, 'counterpartyage_updated', 'days', )); scene.command('counterpartyorders', resetMessage, createRequirementCommand( '/counterpartyorders', 'min_completed_orders', 'MAX_COUNTERPARTY_ORDERS_REQUIREMENT', 10, 'counterpartyorders_updated', 'orders', ));🤖 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 `@bot/modules/user/scenes/settings.ts` around lines 144 - 243, The `counterpartyage` and `counterpartyorders` handlers in `scene.command(...)` duplicate the same parse/validate/cap/save/feedback flow, so extract that shared logic into a reusable factory/helper and have both commands call it with their specific command name, target field, env var, max default, and i18n key. Use the existing `CommunityContext`, `CommunityWizardState`, `readNonNegativeInt`, and `updateMessage` flow as the shared core, and keep only the per-command differences in the thin wrappers to avoid future drift.
🤖 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 `@bot/modules/user/scenes/settings.ts`:
- Around line 12-15: The helper readNonNegativeInt currently treats a blank
string as 0 because Number('') returns 0, so blank env vars like
MAX_COUNTERPARTY_AGE_REQUIREMENT will bypass the fallback. Update
readNonNegativeInt in settings.ts to explicitly treat undefined and
empty/whitespace-only strings as missing and return the fallback before calling
Number, while keeping the integer/non-negative validation for real values.
In `@locales/de.yaml`:
- Around line 254-257: The /settings help text is using escaped angle brackets,
so the placeholder names will show incorrectly under Markdown parsing. Update
the localized strings in the German locale for the counterparty requirement
commands to use raw <Tage> and <Aufträge> text, matching how bot/messages.ts
sends help with parse_mode: Markdown.
In `@locales/en.yaml`:
- Around line 258-262: The /help text in the locales/en.yaml entry is using
escaped placeholders for the counterparty commands, so ctx.reply with Markdown
will render them literally. Update the help strings for the /counterpartyage and
/counterpartyorders entries in the localized help content to use raw angle
brackets (<days> and <orders>) so the placeholders display consistently with the
rest of the help text.
In `@locales/es.yaml`:
- Around line 652-667: The Spanish locale strings in es.yaml mix voseo with the
file’s otherwise consistent tuteo. Update the messages in this block, especially
not_meeting_requirements and invalid_range, to use the same tuteo register as
the rest of the locale entries. Keep the wording natural and consistent with
existing strings like the ones used in the main bot flow, and verify any nearby
related messages in the same counterparty requirements section for the same
mismatch.
In `@locales/fa.yaml`:
- Around line 286-289: The help text in the locale entry for the /settings
commands is using escaped placeholders like <روز> and <سفارش> while
the message is sent with parse_mode Markdown, so update this block to use the
same raw angle-bracket placeholders used elsewhere. Make the change in the help
text content itself and mirror it in the duplicated help block below, keeping
the command names /counterpartyage, /counterpartyorders, and /resetrequirements
unchanged.
In `@locales/fr.yaml`:
- Around line 648-663: The order-flow strings in locales/fr.yaml mix formal and
informal address, so update the affected translation keys to match the existing
“tu” register used elsewhere in the order-taking flow. Specifically, revise
not_meeting_requirements and invalid_range to use informal phrasing consistent
with messages like the self-order warning, and keep the wording aligned with the
existing counterparty-related keys in this section.
In `@locales/it.yaml`:
- Around line 254-257: The settings help text in the locale entry contains
unescaped placeholder brackets, so the HTML-rendered `user_settings` view may
treat them as tags. Update the affected help strings in the `locales/it.yaml`
settings section so the `/setaddress` placeholder and the other command
placeholders use `<...>` consistently, matching the existing escaped
format used by the surrounding help text.
In `@locales/ko.yaml`:
- Around line 255-258: The `/help` text in the Korean locale is using escaped
angle-bracket placeholders, so update the help strings under the counterparty
requirements section to use raw placeholder syntax consistent with the rest of
the help output. Adjust the localized entries in the relevant translation block
so `/counterpartyage` and `/counterpartyorders` display placeholders like `<일>`
and `<주문>` when rendered by the help formatter.
In `@locales/pt.yaml`:
- Around line 255-258: The /help text in the localization entry uses escaped
angle brackets for the /counterpartyage and /counterpartyorders placeholders,
which will render incorrectly under Markdown parsing. Update the strings in the
Portuguese locale so the help content uses plain angle brackets for the
placeholder names, keeping the surrounding text in the same localized section.
In `@locales/ru.yaml`:
- Around line 253-256: The help text in the ru locale uses HTML entities for the
parameter placeholders, which will be shown literally under Markdown parsing.
Update the localized strings around the counterparty settings help entries in
locales/ru.yaml to use raw angle brackets for the placeholders in the same style
as the other /help lines, keeping the existing command names like
/counterpartyage, /counterpartyorders, and /resetrequirements unchanged.
In `@locales/uk.yaml`:
- Around line 253-256: The Ukrainian help text is using HTML-escaped placeholder
brackets, so `/counterpartyage` and `/counterpartyorders` will render
incorrectly under the Markdown help flow. Update the `help` entry in
`locales/uk.yaml` to use plain angle brackets for the placeholders, keeping the
existing command wording intact so `ctx.i18n.t('help')` in `bot/messages.ts`
displays them correctly.
---
Nitpick comments:
In `@bot/modules/orders/takeOrder.ts`:
- Around line 161-188: Add unit tests for meetsCounterpartyRequirements in
takeOrder.ts to cover the new gate behavior in both buy/sell take flows. Verify
the min_days_using_bot and min_completed_orders thresholds block and allow
correctly, confirm 0 disables each check, and ensure the function returns true
when orderCreator.counterparty_requirements is missing. Use mocks for getUserAge
and messages.notMeetingRequirementsMessage to assert the right branch behavior.
In `@bot/modules/user/scenes/settings.ts`:
- Around line 154-155: The local variables in the settings scene are using
snake_case instead of the repo’s required camelCase style. Update the variable
names in the parsing/validation logic in the settings flow, including the
`min_days` and `min_orders` values around the `parseInt` checks in
`settings.ts`, to use camelCase such as `minDays` and `minOrders`, and make sure
any subsequent references in the same methods are renamed consistently.
- Around line 144-243: The `counterpartyage` and `counterpartyorders` handlers
in `scene.command(...)` duplicate the same parse/validate/cap/save/feedback
flow, so extract that shared logic into a reusable factory/helper and have both
commands call it with their specific command name, target field, env var, max
default, and i18n key. Use the existing `CommunityContext`,
`CommunityWizardState`, `readNonNegativeInt`, and `updateMessage` flow as the
shared core, and keep only the per-command differences in the thin wrappers to
avoid future drift.
🪄 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: c1cb1e76-aef4-45cf-9bec-8391fcdf00dc
📒 Files selected for processing (15)
.env-samplebot/messages.tsbot/modules/orders/takeOrder.tsbot/modules/user/scenes/settings.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamlmodels/user.ts
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review
Verified locally: tsc clean, lint clean, full test suite passes (144 passing), all 10 locales contain the 6 new keys plus the user_settings and help additions, and the Mongoose nested-subdocument schema is correct (optional subdoc, defaults apply only when set — old users are unaffected). The enforcement runs inside PerOrderIdMutex, and both take paths (channel buttons and /takeorder command) route through takebuy/takesell, so there is no bypass. The env caps (MAX_COUNTERPARTY_*) prevent griefing via absurd requirements. Overall the feature is well contained.
Findings, by severity:
1. HIGH — No test coverage for the new enforcement logic
meetsCounterpartyRequirements is a pure gate over getUserAge + trades_completed and is trivially unit-testable (the repo already stubs getUserAge in tests/bot/bot.spec.ts). The settings command handlers (parsing, range validation vs env caps, reset) are also untested. This is order-taking enforcement — a regression here silently blocks (or stops blocking) trades. Please add specs for: requirement met/unmet on both takebuy and takesell, counterparty_requirements absent (legacy users), boundary values (age == min, orders == min), and the range/NaN validation in the settings commands.
2. MEDIUM — Requirements check runs before order-state validation
In both takebuy and takesell the new check executes before validateTakeBuyOrder/validateTakeSellOrder. If the order is no longer takeable (e.g. taken by someone else in the race window inside the mutex), a taker who doesn't meet the requirements receives "you do not meet the counterparty requirements" when the true state is "order not available". Moving the check after the order-state validation gives correct error precedence at zero cost.
3. MEDIUM — Generic denial message gives the taker no actionable information
not_meeting_requirements doesn't say which requirement failed nor the thresholds (min_days_using_bot, min_completed_orders are right there when the check fails). Since published orders don't display the maker's requirements, the only way a taker discovers them is by failing with a generic message. Suggest interpolating the failed requirement and its threshold into the message — the i18n plumbing (ctx.i18n.t(key, params)) already supports it.
4. LOW — Loose user-input parsing, inconsistent with the env parsing
parseInt(days) accepts "5abc" → 5 and "5.9" → 5 silently, while readNonNegativeInt (used for the env caps) correctly requires a strict integer. Reuse the same strict validation for user input (and add the radix if you keep parseInt).
5. LOW — ~90 duplicated lines between counterpartyage and counterpartyorders
The two handlers differ only in: field name, env var, fallback max, and feedback key. A single factory makeRequirementCommand({field, envVar, fallbackMax, feedbackKey, paramKey}) removes the duplication and keeps future changes in one place. Same for the duplicated default object {min_days_using_bot: 0, min_completed_orders: 0} (appears 3×) and the magic fallbacks 30/10 duplicated against .env-sample — extract named constants.
6. LOW — Legacy users without created_at pass the age check via NaN
getUserAge on a missing created_at yields NaN, and NaN < min_days_using_bot is false, so the user passes. Probably acceptable (such accounts are old), but worth a comment or an explicit guard so it's a decision, not an accident.
Design questions (non-blocking)
- The requirements also apply inside communities, on top of community bans and any community policy. Intended?
resetrequirementsstores{0, 0}instead of$unset-ing the subdoc — harmless, but leaves residual data on every user who ever touched the feature.- A maker can set requirements they themselves don't meet (asymmetric by design, presumably fine — just flagging).
CI is green and the branch is mergeable. My only blocking concern is #1 given this gates order-taking; #2/#3 are cheap to fix in this PR.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Requesting changes per my detailed review above: the blocking item is HIGH #1 — no test coverage for meetsCounterpartyRequirements and the new settings commands, since this logic gates order-taking. #2 (requirements check runs before order-state validation) and #3 (generic denial message) are cheap to address in the same pass. Happy to re-review once tests are added.
…he doesn't meets the requirements of the counterparty * remove code duplication when declaring the /counterpartyage and /counterpartyorders in settings.ts * added a test case for takeOrder validating the new counterparty requirements restriction
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bot/modules/orders/takeOrder.ts (1)
161-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: de-duplicate the denial message call.
The
messages.notMeetingRequirementsMessage(ctx, user, { min_days_using_bot, min_completed_orders })block is repeated verbatim in both requirement branches. Extracting the params once keeps the two failure paths in sync.♻️ Proposed refactor
const { min_days_using_bot, min_completed_orders } = orderCreator.counterparty_requirements; + const requirements = { min_days_using_bot, min_completed_orders }; if (min_days_using_bot > 0) { const ageInDays = getUserAge(user); // Legacy accounts without a created_at yield NaN here. Such accounts // predate created_at tracking and are genuinely old, so we let them pass // the age check explicitly instead of relying on NaN comparisons. if (!Number.isNaN(ageInDays) && ageInDays < min_days_using_bot) { - await messages.notMeetingRequirementsMessage(ctx, user, { - min_days_using_bot, - min_completed_orders, - }); + await messages.notMeetingRequirementsMessage(ctx, user, requirements); return false; } } if (min_completed_orders > 0) { if (user.trades_completed < min_completed_orders) { - await messages.notMeetingRequirementsMessage(ctx, user, { - min_days_using_bot, - min_completed_orders, - }); + await messages.notMeetingRequirementsMessage(ctx, user, requirements); return false; } }🤖 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 `@bot/modules/orders/takeOrder.ts` around lines 161 - 196, The denial message call in meetsCounterpartyRequirements is duplicated in both the min_days_using_bot and min_completed_orders failure paths. Extract the repeated messages.notMeetingRequirementsMessage(ctx, user, { min_days_using_bot, min_completed_orders }) arguments into a shared variable or helper within meetsCounterpartyRequirements, and reuse it in both branches to keep the two checks in sync.
🤖 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.
Nitpick comments:
In `@bot/modules/orders/takeOrder.ts`:
- Around line 161-196: The denial message call in meetsCounterpartyRequirements
is duplicated in both the min_days_using_bot and min_completed_orders failure
paths. Extract the repeated messages.notMeetingRequirementsMessage(ctx, user, {
min_days_using_bot, min_completed_orders }) arguments into a shared variable or
helper within meetsCounterpartyRequirements, and reuse it in both branches to
keep the two checks in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5da7480b-219a-4e83-b545-84780b2364ae
📒 Files selected for processing (14)
bot/messages.tsbot/modules/orders/takeOrder.tsbot/modules/user/scenes/settings.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamltests/bot/modules/orders/takeOrder.spec.ts
✅ Files skipped from review due to trivial changes (6)
- locales/ko.yaml
- locales/ru.yaml
- locales/es.yaml
- locales/fa.yaml
- locales/pt.yaml
- locales/it.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- bot/messages.ts
- locales/uk.yaml
- locales/en.yaml
- locales/de.yaml
- locales/fr.yaml
- bot/modules/user/scenes/settings.ts
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review of this PR. The structure is solid: all 10 locales define the new keys, both take entry points (text commands and callback buttons) converge on the gated takebuy/takesell so no path bypasses the check, the correct party's requirements are enforced in both flows (buy-order creator via buyer_id, sell-order creator via seller_id), and the tests cover the boundary and legacy-NaN cases.
7 findings, most-severe first — details in the inline comments:
- 🐛
settings.ts— a double space between command and value silently stores0and disables the requirement (Number('') === 0). takeOrder.ts+ locales — the denial message always cites both thresholds, so a taker is told to meet a requirement of "0" that was never set.settings.ts— repeating a command with the same value gives no visible acknowledgement (themessageChangedguard skips the edit and the user's command was already deleted).takeOrder.ts— the gate is called besidevalidateTakeBuyOrder/validateTakeSellOrderinstead of living inside them; future take paths reusing the validators would silently skip it.takeOrder.ts— the legacy-account NaN guard belongs ingetUserAgeitself; the other 5 callers of that helper render a literal "NaN" for the same accounts.settings.ts— the non-negative-integer rule is duplicated within the file, andNumber()-based env parsing diverges from the repo'sparseInt(process.env.X || 'N', 10)idiom.settings.ts—/resetrequirementsstores zeros instead of unsetting the field, creating two representations of "no requirements".
Findings 1–3, 6 and 7 are fixed (with the gate test updated for per-threshold denial messages) on branch claude/pr-853-strict-review-uxjmvl — lint clean and 151 tests passing — in case you want to cherry-pick or merge it into this branch. 4 and 5 are left as design suggestions since they involve refactor/product decisions.
🤖 Generated with Claude Code
Generated by Claude Code
| const [, value] = ctx.message.text.trim().split(' '); | ||
| const parsed = Number(value); |
There was a problem hiding this comment.
Bug (high): a double space silently stores 0 and disables the requirement.
"/counterpartyage 30".trim().split(' ') yields ['/counterpartyage', '', '30'], so value binds to '' and Number('') === 0, which passes the Number.isInteger(parsed) && parsed >= 0 check. The bot replies "updated to 0 days" and the user — who typed 30 — ends up with the gate disabled while believing it's active.
Splitting on runs of whitespace fixes it:
| const [, value] = ctx.message.text.trim().split(' '); | |
| const parsed = Number(value); | |
| const [, value] = ctx.message.text.trim().split(/\s+/); | |
| const parsed = Number(value); |
Generated by Claude Code
| await messages.notMeetingRequirementsMessage(ctx, user, { | ||
| min_days_using_bot, | ||
| min_completed_orders, | ||
| }); |
There was a problem hiding this comment.
UX: the denial message always cites both thresholds, including the one that was never set.
If the maker only ran /counterpartyage 30 (so min_completed_orders is 0), a rejected taker receives:
"…requires at least 30 days using the bot and 0 completed orders."
The "0 completed orders" clause is confusing noise — it presents an unset requirement as if it were configured. Same in reverse when only the orders minimum is set ("at least 0 days using the bot and 5 completed orders").
Suggestion: split not_meeting_requirements into two keys (not_meeting_age_requirement / not_meeting_orders_requirement) and send only the threshold that actually failed. This also removes the duplicated notMeetingRequirementsMessage(...); return false; block in this function.
Generated by Claude Code
| await user.save(); | ||
| state.feedback = { i18n: feedbackKey, [paramKey]: parsed }; |
There was a problem hiding this comment.
UX: repeating a command with the same value produces no visible response at all.
Sequence: user runs /counterpartyage 5 (message updates, shows the "updated to 5 days" feedback line), then runs /counterpartyage 5 again. resetMessage only deletes state.feedback from state — it doesn't re-render — so the displayed message still contains the old feedback line. The handler re-saves and sets identical feedback, updateMessage recomposes a byte-identical string, the messageChanged guard skips editMessageText, and ctx.deleteMessage() has already removed the user's command. Net result: the command vanishes with zero acknowledgement.
Fix: have resetMessage re-render (without the previous feedback/error) before calling next(), so the post-command updateMessage always differs from the displayed text.
Generated by Claude Code
|
|
||
| if (!(await validateTakeBuyOrder(ctx, bot, user, order))) return; | ||
|
|
||
| if (!(await meetsCounterpartyRequirements(ctx, user, userOffer))) return; |
There was a problem hiding this comment.
Design: the gate sits beside the shared take validators instead of inside them.
All sibling take-path checks (own order, order type, PENDING status…) live in validateTakeBuyOrder/validateTakeSellOrder in bot/validations.ts (also re-exported from bot/index.ts). The new counterparty rule is conceptually one of those checks but is called separately at two call sites. Today no path bypasses it — text commands and callback buttons both converge on takebuy/takesell — but any future take path that reuses the validators (a new scene, a community-side take) will pass validation and silently skip this rule.
Not blocking (it would require loading the order creator inside the validators), but worth considering moving the check into the validators so every take path gets it by construction.
Generated by Claude Code
| const ageInDays = getUserAge(user); | ||
| // Legacy accounts without a created_at yield NaN here. Such accounts | ||
| // predate created_at tracking and are genuinely old, so we let them pass | ||
| // the age check explicitly instead of relying on NaN comparisons. | ||
| if (!Number.isNaN(ageInDays) && ageInDays < min_days_using_bot) { |
There was a problem hiding this comment.
Design: the NaN guard is one level too shallow — it belongs in getUserAge.
getUserAge has 6 callers and only this new one guards NaN. The others interpolate the raw result into user-facing text: getDetailedOrder (util/index.ts:431), bot/messages.ts:90 and :537, bot/ordersActions.ts:257, and the Nostr event tag (bot/modules/nostr/events.ts:27) all render a literal NaN for the same legacy accounts this comment describes. Handling invalid created_at inside getUserAge itself would cover every caller and remove this special case.
The catch is picking a return value that works for both display and comparison call sites, which is a product decision — flagging it rather than prescribing one. Fine to defer to a follow-up, but the helper is the right place for the fix.
Generated by Claude Code
| const readNonNegativeInt = (value: string | undefined, fallback: number) => { | ||
| if (value === undefined || value.trim() === '') return fallback; | ||
| const parsed = Number(value); | ||
| return Number.isInteger(parsed) && parsed >= 0 ? parsed : fallback; | ||
| }; |
There was a problem hiding this comment.
Cleanup: the non-negative-integer rule is written twice in this file, and the env parsing diverges from the codebase idiom.
Number.isInteger(parsed) && parsed >= 0here is duplicated (negated) insidemakeRequirementCommand(!Number.isInteger(parsed) || parsed < 0). Extracting a sharedisNonNegativeIntpredicate keeps the two paths from drifting.- Elsewhere the repo reads numeric env vars with
parseInt(process.env.X || 'N', 10)(bot/middleware/commandlogging.ts:7,bot/modules/community/scenes.ts:948,models/community.ts:4).Number()behaves differently on the same input (parseInt('10abc', 10) === 10vsNumber('10abc')→ NaN), so identical-looking config values parse under different rules depending on the variable.Number()is arguably the safer of the two — just noting the inconsistency so it's a deliberate choice.
Generated by Claude Code
| user.counterparty_requirements = { | ||
| ...DEFAULT_COUNTERPARTY_REQUIREMENTS, | ||
| }; |
There was a problem hiding this comment.
Cleanup: reset creates a second representation of "no requirements".
After /resetrequirements, "no requirements" exists in two encodings: undefined (never configured) and {min_days_using_bot: 0, min_completed_orders: 0} (reset). The gate's fast path if (!orderCreator.counterparty_requirements) return true only recognizes the first; the reset user takes the slow path instead (same outcome today, since the > 0 guards don't fire, but any future logic that branches on the field's presence will treat the two differently).
Simpler to unset the field on reset:
| user.counterparty_requirements = { | |
| ...DEFAULT_COUNTERPARTY_REQUIREMENTS, | |
| }; | |
| user.counterparty_requirements = undefined; | |
| await user.save(); |
Generated by Claude Code
3 participants
Continues #827
Summary by CodeRabbit
/settingscommands to update, reset, and enforce counterparty requirements for order taking (buy and sell).