feat: add optional [community] param to /buy and /sell commands - #879
feat: add optional [community] param to /buy and /sell commands#879Matobi98 wants to merge 3 commits into
Conversation
WalkthroughBuy and sell commands accept an optional community name, resolve explicit communities, extend order validation, add routing coverage, and document the new syntax across supported locales. ChangesCommunity-targeted orders
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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.
🧹 Nitpick comments (1)
bot/validations.ts (1)
200-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unreachable validation blocks.
Since
parseOptionalOrderParamsonly assignspriceMarginwhen the argument is a valid numeric string,priceMarginwill never evaluate totrueforisNaN(priceMargin). This makes these explicit validation blocks, which were left over from the previous positional parsing logic, unreachable dead code.
bot/validations.ts#L200-L207: Remove this dead code block fromvalidateSellOrder.bot/validations.ts#L294-L301: Remove this dead code block fromvalidateBuyOrder.🤖 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/validations.ts` around lines 200 - 207, Remove the unreachable priceMargin isNaN validation block from validateSellOrder at bot/validations.ts:200-207 and from validateBuyOrder at bot/validations.ts:294-301. Leave the surrounding validation flow unchanged.
🤖 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/validations.ts`:
- Around line 200-207: Remove the unreachable priceMargin isNaN validation block
from validateSellOrder at bot/validations.ts:200-207 and from validateBuyOrder
at bot/validations.ts:294-301. Leave the surrounding validation flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b807f760-4234-41f1-984e-69c5400d17d6
📒 Files selected for processing (14)
bot/modules/orders/commands.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/communityHelper.ts
There was a problem hiding this comment.
Strict review summary
Verdict: Request changes
Two blocking input/routing regressions remain:
- Numeric Telegram group IDs are classified as price margins, so a valid community identifier can fall back to the user's default community and be persisted as an invalid margin.
- In group chats, a trailing non-numeric token is parsed as a community and then ignored, so an unquoted multi-word payment method can now publish a silently truncated order instead of being rejected.
Also, the linked issue asks to publish to the explicit community alongside the currently selected one, while the current implementation selects only one destination. Please clarify or implement that intended behavior before closing #877.
The CI checks are green and all locale YAML files parse successfully; these blockers are behavioral.
There was a problem hiding this comment.
Additional strict-review findings
A second independent pass found two more routing/access blockers in the display-name fallback. These are in addition to the parsing issues from the previous review:
- Case-insensitive name matching is ambiguous because the schema only enforces case-sensitive uniqueness.
- Name fallback can target private communities without proving membership or knowledge of the canonical group identifier.
The new routing tests should cover these cases and assert the final community_id, not only the parsed arguments.
| const escaped = identifier.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| const regex = new RegExp(`^${escaped}$`, 'i'); | ||
|
|
||
| const community = |
There was a problem hiding this comment.
🔴 Blocking access regression: the display-name fallback searches every enabled community, including public: false, without membership/ownership authorization. Previously, selecting a private community through /setcomm required its canonical @group or private numeric group ID. Now an unbanned user who only guesses the display name can publish into that private community's order channel. Restrict name fallback to public/authorized communities, or require the canonical group identifier for private ones.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/util/communityHelper.spec.ts (2)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
requirewith ES6 imports.As per coding guidelines, TypeScript files should adhere to standard ESLint and TypeScript rules (
@typescript-eslint/no-var-requires). Please useimportstatements instead ofrequire.♻️ Proposed refactor
-const { expect } = require('chai'); -const sinon = require('sinon'); -const { Community } = require('../../models'); -const { getCommunityByIdentifier } = require('../../util/communityHelper'); +import { expect } from 'chai'; +import sinon from 'sinon'; +import { Community } from '../../models'; +import { getCommunityByIdentifier } from '../../util/communityHelper';🤖 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/util/communityHelper.spec.ts` around lines 3 - 6, Replace the CommonJS require declarations at the top of communityHelper.spec.ts with equivalent ES6 import statements for expect, sinon, Community, and getCommunityByIdentifier, preserving their current bindings and usage.Source: Coding guidelines
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid using
anytype.As per coding guidelines, follow standard TypeScript rules and prevent explicitly declaring
any(@typescript-eslint/no-explicit-any). Ifuseris a partial mock, you can omit the explicit typing or cast it selectively without bypassing the rule globally.♻️ Proposed refactor
- const user: any = { _id: 'user-1' }; + const user = { _id: 'user-1' };🤖 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/util/communityHelper.spec.ts` at line 18, Update the user mock declaration in communityHelper.spec.ts to remove the explicit any type, using inferred typing or a narrow selective cast that satisfies the expected user shape without disabling the no-explicit-any rule.Source: Coding guidelines
bot/validations.ts (1)
174-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid using
anytype forpriceMargin.As per coding guidelines, TypeScript ESLint rules should be followed and warnings shouldn't be bypassed (
@typescript-eslint/no-explicit-any). Theanytype here defeats type safety and isn't necessary, as standardstringvalues are compatible with downstream parsing functions.♻️ Proposed refactor
- const priceMargin: any = optionalArgs[0] === '' ? undefined : optionalArgs[0]; + const priceMargin: string | undefined = optionalArgs[0] === '' ? undefined : optionalArgs[0];🤖 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/validations.ts` at line 174, Remove the explicit any annotation from priceMargin in the validation logic, allowing TypeScript to infer or use the appropriate string/undefined type. Preserve the existing empty-string-to-undefined behavior and ensure downstream parsing continues to accept the resulting value.Source: Coding guidelines
🤖 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/validations.ts`:
- Line 174: Remove the explicit any annotation from priceMargin in the
validation logic, allowing TypeScript to infer or use the appropriate
string/undefined type. Preserve the existing empty-string-to-undefined behavior
and ensure downstream parsing continues to accept the resulting value.
In `@tests/util/communityHelper.spec.ts`:
- Around line 3-6: Replace the CommonJS require declarations at the top of
communityHelper.spec.ts with equivalent ES6 import statements for expect, sinon,
Community, and getCommunityByIdentifier, preserving their current bindings and
usage.
- Line 18: Update the user mock declaration in communityHelper.spec.ts to remove
the explicit any type, using inferred typing or a narrow selective cast that
satisfies the expected user shape without disabling the no-explicit-any rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7d900657-19a7-4a4b-8b62-4856348da5bd
📒 Files selected for processing (15)
bot/modules/orders/commands.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.tstests/util/communityHelper.spec.tsutil/communityHelper.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- locales/de.yaml
- locales/fa.yaml
- locales/ru.yaml
- locales/pt.yaml
- util/communityHelper.ts
- locales/es.yaml
- bot/modules/orders/commands.ts
- locales/it.yaml
- locales/ko.yaml
- locales/fr.yaml
- locales/en.yaml
- locales/uk.yaml
There was a problem hiding this comment.
Strict re-review — current head
Reviewed the complete PR conversation and re-checked head 15406a952f703c48758a11724399c229937deaf6.
Fixed since the previous review
- Numeric Telegram group IDs are now preserved by positional parsing.
- Unquoted multi-word payment methods fail closed again.
- Explicit communities are honored in both private chats and groups.
- Display-name lookup is exact and public-only; private communities require the canonical group identifier.
Remaining blockers
- The implementation still does not satisfy the linked issue's core workflow: #877 explicitly requires publishing to the optional community alongside the currently selected community, while this PR creates one order for one destination and tells the user to run the command again. Either implement the requested behavior or explicitly realign the issue/PR contract before this can close #877.
- The new tests stop at validator/helper output. They do not execute the
/buyor/sellhandler throughresolveOrderCommunityand assert thecommunity_idpassed tocreateOrder. That integration is where the previous private-vs-group routing bug existed, so it needs focused coverage on the production path.
CI is green. TypeScript compilation, ESLint, Prettier, and the three new helper tests also pass locally. The full local validation suite could not load because canvas.node is unavailable in this environment; GitHub CI runs the full suite successfully.
Verdict: Request changes.
| await ctx.reply(ctx.i18n.t('community_not_found')); | ||
| return null; | ||
| } | ||
| return communityInfo; |
There was a problem hiding this comment.
🔴 Blocking scope mismatch: this returns a single resolved destination, and each handler calls createOrder only once. However, #877 explicitly asks for the optional community to be published alongside the currently selected community; requiring the user to run the command again preserves the workflow the issue was meant to remove. Please either implement the two-destination behavior or explicitly realign the issue/PR contract instead of closing #877.
There was a problem hiding this comment.
This PR intentionally publishes to a single destination. Double-publishing to two communities at once was considered too complex and out of scope. The [community] param lets the user choose the destination explicitly; publishing to a second community means running the command again. We're realigning the scope: this PR does not fully close #877, it delivers the optional-community parameter only.
|
|
||
| const result = await getCommunityByIdentifier(user, '-1001234567890'); | ||
|
|
||
| expect(String(result.communityId)).to.equal('community-private'); |
There was a problem hiding this comment.
🔴 Blocking coverage gap: this asserts the helper result, not the final community_id passed by /buy or /sell to createOrder. The previous group-chat bug was in the integration between parsing and resolveOrderCommunity, and all these helper tests would pass with that bug still present. Please add focused command-path tests for private and group contexts (including numeric IDs, explicit destination precedence, ban/currency rejection) and assert the createOrder payload.
There was a problem hiding this comment.
Strict re-review after the latest conversation
I re-read the complete PR conversation, including the author's reply on the scope thread, and re-checked current head 15406a952f703c48758a11724399c229937deaf6.
The author's clarification makes the single-destination behavior acceptable as a narrower feature; I am no longer asking this PR to implement double publication. However, the PR body still contains Closes #877, while both the reply and the implementation explicitly do not fulfill #877's additive-publication requirement. Merging this PR would therefore auto-close an issue that remains unresolved. Please remove/replace the closing keyword and track the remaining #877 behavior separately.
The production-path coverage blocker is also unchanged: the added tests call validators and getCommunityByIdentifier directly, but never execute /buy or /sell through resolveOrderCommunity and assert the final community_id passed to createOrder. The earlier group-routing regression would still pass the current tests. Please add focused command-handler tests for private/group contexts, explicit precedence, numeric IDs, unknown communities, bans, and unsupported currencies.
Previous parser, visibility, and routing correctness blockers remain fixed. CI is green and git diff --check passes.
Readability
The intent is much clearer after the added positional-parsing and routing comments. The remaining any/CommonJS test nits are already covered by CodeRabbit and are non-blocking; the PR description itself should also be cleaned up because several sentences are currently truncated/mangled.
Verdict: Request changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/bot/orders-commands.spec.ts (3)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror the runtime module layout for this spec.
The handler under test is
bot/modules/orders/commands.ts, but this spec is located attests/bot/orders-commands.spec.ts. Move it totests/bot/modules/orders/commands.spec.tsso tests follow the required runtime layout.As per coding guidelines, specs must be under
tests/, use the.spec.tssuffix, and mirror the runtime source layout.🤖 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/orders-commands.spec.ts` around lines 13 - 21, Move the command-path integration spec containing “/buy and /sell community routing (command path)” from the current orders test location into the mirrored tests/bot/modules/orders/commands.spec.ts location, preserving its .spec.ts suffix and test content.Source: Coding guidelines
34-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake validator stubs command-specific.
Both validators return the same params, so a regression where
/buycallsvalidateSellOrder(or vice versa) would still pass. Configure only the expected validator per test, or assert the correct validator was called and the other was not.🤖 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/orders-commands.spec.ts` around lines 34 - 36, Update the validator stubs in the order command tests around validateBuyOrder and validateSellOrder so each test configures only the validator expected for that command, and verifies the opposite validator is not called. Ensure /buy specifically exercises validateBuyOrder and /sell specifically exercises validateSellOrder.
100-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the explicit
/selltest creates a sell order.This test verifies the selected community but would pass if the handler submitted
type: 'buy'. AddcreateOrder.calledOnceand assertcreateOrder.firstCall.args[3].type === 'sell'.Proposed assertion
expect(communityHelper.getCommunityInfo.called).to.equal(false); + expect(createOrder.calledOnce).to.equal(true); expect(createOrder.firstCall.args[3].community_id).to.equal( 'community-explicit', ); + expect(createOrder.firstCall.args[3].type).to.equal('sell');🤖 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/orders-commands.spec.ts` around lines 100 - 123, The explicit community `/sell` test must also verify that a sell order is created. In the test case around `sell`, assert `createOrder` is called exactly once and validate that the first call’s order argument has `type` set to `sell`, while preserving the existing community assertions.
🤖 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/orders-commands.spec.ts`:
- Around line 40-44: Update the test setup around beforeEach so
MAX_PENDING_ORDERS is always assigned a deterministic value rather than
preserving an ambient one, and capture its original value before overriding it.
Add or update afterEach to restore the original environment variable, including
deleting it when it was initially unset, so each test remains isolated and
handler routing is exercised reliably.
---
Nitpick comments:
In `@tests/bot/orders-commands.spec.ts`:
- Around line 13-21: Move the command-path integration spec containing “/buy and
/sell community routing (command path)” from the current orders test location
into the mirrored tests/bot/modules/orders/commands.spec.ts location, preserving
its .spec.ts suffix and test content.
- Around line 34-36: Update the validator stubs in the order command tests
around validateBuyOrder and validateSellOrder so each test configures only the
validator expected for that command, and verifies the opposite validator is not
called. Ensure /buy specifically exercises validateBuyOrder and /sell
specifically exercises validateSellOrder.
- Around line 100-123: The explicit community `/sell` test must also verify that
a sell order is created. In the test case around `sell`, assert `createOrder` is
called exactly once and validate that the first call’s order argument has `type`
set to `sell`, while preserving the existing community assertions.
🪄 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 Plus
Run ID: a5a2e0e3-702d-445a-90ef-744399b1fa0c
📒 Files selected for processing (1)
tests/bot/orders-commands.spec.ts
| beforeEach(() => { | ||
| // isMaxPending -> not maxed out | ||
| sinon.stub(Order, 'countDocuments').resolves(0); | ||
| process.env.MAX_PENDING_ORDERS = process.env.MAX_PENDING_ORDERS || '5'; | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Isolate MAX_PENDING_ORDERS from the test environment.
Using || preserves any ambient value, and afterEach never restores it. A CI or neighboring test value can cause handlers to exit through the pending-order guard before routing is exercised. Set a deterministic value and restore the original environment variable afterward.
Proposed fix
+ let previousMaxPendingOrders: string | undefined;
+
beforeEach(() => {
+ previousMaxPendingOrders = process.env.MAX_PENDING_ORDERS;
- process.env.MAX_PENDING_ORDERS = process.env.MAX_PENDING_ORDERS || '5';
+ process.env.MAX_PENDING_ORDERS = '5';
...
- afterEach(() => sinon.restore());
+ afterEach(() => {
+ sinon.restore();
+ if (previousMaxPendingOrders === undefined) {
+ delete process.env.MAX_PENDING_ORDERS;
+ } else {
+ process.env.MAX_PENDING_ORDERS = previousMaxPendingOrders;
+ }
+ });Also applies to: 67-67
🤖 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/orders-commands.spec.ts` around lines 40 - 44, Update the test
setup around beforeEach so MAX_PENDING_ORDERS is always assigned a deterministic
value rather than preserving an ambient one, and capture its original value
before overriding it. Add or update afterEach to restore the original
environment variable, including deleting it when it was initially unset, so each
test remains isolated and handler routing is exercised reliably.
There was a problem hiding this comment.
Strict re-review — current head
I re-read the complete GitHub conversation and reviewed the delta from 15406a952f703c48758a11724399c229937deaf6 to current head c8371680779bfdd4809dd1fe46eeb9264fba72b4.
Fixed
The previous production-path coverage blocker is resolved. The new command-handler tests execute buy/sell, verify explicit-vs-fallback routing in private and group chats, preserve numeric identifiers, assert the final community_id passed to createOrder, and cover banned, unsupported-currency, and not-found exits. The earlier group-routing regression would now fail these tests.
All earlier parser, visibility, lookup, and routing blockers remain fixed.
Remaining blocker
The PR body still contains Closes #877. The author has explicitly clarified that this PR intentionally implements one destination and does not fully implement #877, whose requested behavior is additive publication. Merging with this keyword would auto-close an unresolved issue. Please remove/replace the closing keyword and keep #877 (or a follow-up issue) open for the remaining behavior.
Readability
The new test names and routing comments are clear. CodeRabbit's test-layout, any/CommonJS, validator-specific stub, sell-type assertion, and environment-restoration notes are worthwhile but non-blocking. The PR description still contains several truncated/mangled sentences and should be cleaned up while correcting the issue linkage.
CI is green. TypeScript compilation, ESLint, Prettier, and git diff --check pass locally.
Verdict: Request changes.
grunch
left a comment
There was a problem hiding this comment.
Thanks for this — the foundation is solid. Case-sensitive name matching restricted to public communities is the right call, the regex escaping is correctly implemented, and the localization coverage is exemplary: all 10 languages have the complete note with correct MarkdownV2 escaping. A few things need addressing before merge.
Verification performed locally on this branch
| Check | Result |
|---|---|
tsc -p tsconfig.test.json --noEmit |
✅ exit 0 |
npm run lint |
✅ clean |
| New/changed specs from this PR | ✅ 16/16 pass (7 orders-commands, 3 communityHelper, 6 in validation) |
npm test (full suite) |
❌ Cannot find module './commands' in dist/bot/modules/block/index.js — reproduced identically on main, pre-existing, not caused by this PR |
community_not_found across all 10 locales |
✅ present |
| "premium first" note across all 10 locales | ✅ present, correctly escaped |
🔴 HIGH
1. community_not_found leaks into group chats and the invoking message is not deleted
bot/modules/orders/commands.ts:196-199
if (communityName) {
const communityInfo = await getCommunityByIdentifier(user, communityName);
if (!communityInfo.community) {
await ctx.reply(ctx.i18n.t('community_not_found')); // ← replies inside the group
return null;
}Because resolveOrderCommunity returns null, sell/buy return before reaching the existing branch:
// commands.ts:66-69 and 132-135
if (ctx.message?.chat.type !== 'private' && !community) {
return ctx.deleteMessage();
}So /buy 1000 2 USD zelle 0 nonexistentCommunity inside a group now:
- makes the bot post a visible error message in the group (current design is silent: delete and say nothing), and
- leaves the user's command message in the group.
That's two junk messages per attempt and a trivial spam surface, since anyone can repeat it. I confirmed there is no global deletion middleware — grep deleteMessage only returns explicit per-handler calls.
Suggested fix — propagate chat context into the error branch:
if (!communityInfo.community) {
if ((ctx.message?.chat.type || 'private') !== 'private') {
await ctx.deleteMessage();
} else {
await ctx.reply(ctx.i18n.t('community_not_found'));
}
return null;
}Worth a test: group chat + unknown community ⇒ deleteMessage called, ctx.reply not called.
2. The case-insensitive regex on group defeats the unique index → collection scan
util/communityHelper.ts:91-97
const escaped = identifier.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const groupRegex = new RegExp(`^${escaped}$`, 'i');
const community = await Community.findOne({ group: groupRegex, enabled: { $ne: false } });MongoDB cannot use an index for a case-insensitive regex, not even an anchored one. Community.group has unique: true (models/community.ts:74) and this query discards it — a COLLSCAN on every /buy and /sell that passes an explicit community.
It is also unnecessary. The schema declares group: { type: String, unique: true, lowercase: true, sparse: true }, so Mongoose already normalizes to lowercase on save — a fact the function's own docstring acknowledges ("the group is stored lowercase and unique"). An exact lowercase match is semantically equivalent, index-backed, and removes the entire regex-escaping surface along with the cost of compiling a regex from a user-controlled identifier of unbounded length.
const community =
(await Community.findOne({
group: identifier.toLowerCase(),
enabled: { $ne: false },
})) ||
(await Community.findOne({ name: identifier, public: true, enabled: { $ne: false } }));Note: getCommunityInfo and setComm share this pattern, but there the input is @${chat.username} (bounded), not an arbitrary user argument on the order-creation hot path.
🟡 MEDIUM
3. Extra trailing args are silently dropped → the order can land in the wrong community
bot/validations.ts:171-178
const priceMargin: any = optionalArgs[0] === '' ? undefined : optionalArgs[0];
const communityName: string | undefined = optionalArgs[1] === '' ? undefined : optionalArgs[1];Only indices 0 and 1 are read; everything after is discarded. Community.name allows spaces (maxlength: 30), and the help text never says the community must be quoted — unlike the payment method, whose examples do show quotes.
/sell 0 2 USD zelle 0 Bitcoin Argentina → communityName = 'Bitcoin', 'Argentina' is lost. If a community named Bitcoin exists, the order is published to the wrong community with no warning whatsoever.
This contradicts the fail-closed reasoning the PR itself applies to unquoted payment methods. Same treatment here:
if (optionalArgs.length > 2) {
// return false and reply with sell_correct_format / buy_correct_format
}At minimum, document the quoting requirement in all 10 locales with an example (... 0 "Bitcoin Argentina").
4. A community ban can be sidestepped from inside the group itself
bot/modules/orders/commands.ts:186-201
The comment states "An explicit community always wins, in private chats and in groups alike". Consequence: a user banned from group B's community can run, inside B, /buy 1000 2 USD zelle 0 @otherCommunity; only the other community's ban list is evaluated and the order is created.
They could already do this via DM, so it isn't privilege escalation — but it does weaken moderation inside the group, which is exactly where admins expect their ban to hold. Commit 1 restricted the parameter to private chats and commit 2 reverted that; the PR description doesn't explain the change.
This deserves an explicit maintainer decision. If the current behavior stays, the group message should at least be deleted (see #1).
Related: the locale text says "publish the order there instead of your default community", which is inaccurate inside a group — there it replaces the group's community, not the user's.
5. getCommunityByIdentifier tests are white-box, and one is tautological
tests/util/communityHelper.spec.ts
All three tests stub Community.findOne and assert on query shape rather than behavior:
expect(groupQuery.group).to.be.an.instanceOf(RegExp);
expect(groupQuery.group.flags).to.contain('i');That assertion blocks the fix for #2: switching to toLowerCase() breaks a test that verifies no functional guarantee. It should assert that Foo, FOO and foo all resolve to the same community — not that the implementation uses a regex.
The third test is tautological:
findOne.onSecondCall().resolves(null); // the stub decides the outcome
const result = await getCommunityByIdentifier(user, 'PrivateName');
expect(result.community).to.equal(null);It passes even if public: true is removed from the implementation. The real guarantee is already covered by the nameQuery.public assertion in test 2.
Missing coverage on new code:
enabled: { $ne: false }— a disabled community must not resolve. Untested.- The metacharacter escaping (
communityHelper.ts:91) — untested. This is the riskiest line in the PR: with no coverage, a regression reintroduces wildcard matching (/buy … 0 .*matching an arbitrary community). - The
isBannedcomputation insidegetCommunityByIdentifier.
6. Test state leaks across spec files
tests/bot/orders-commands.spec.ts:43
process.env.MAX_PENDING_ORDERS = process.env.MAX_PENDING_ORDERS || '5';Set in beforeEach and never restored — afterEach (line 67) only calls sinon.restore(). Mocha runs every spec in one process and this file runs before others alphabetically, so the variable stays contaminated for the rest of the suite and can mask an unrelated failure.
let prevMaxPending: string | undefined;
beforeEach(() => {
prevMaxPending = process.env.MAX_PENDING_ORDERS;
process.env.MAX_PENDING_ORDERS = '5';
});
afterEach(() => {
sinon.restore();
if (prevMaxPending === undefined) delete process.env.MAX_PENDING_ORDERS;
else process.env.MAX_PENDING_ORDERS = prevMaxPending;
});🔵 LOW / NITS
7. Stale comment contradicting the code — commands.ts:180-185 says "When the user passes a community name in a private chat, the order is published to that community", while the inline comment six lines below (:191) says the opposite: "An explicit community always wins, in private chats and in groups alike". Leftover from commit 1 — please drop the top block.
8. Dead === '' branch — validations.ts:174,176. bot/middleware/commands.ts:42 already runs args = args.filter(arg => arg), so no element can be ''. The guard never fires and implies a defense that doesn't exist.
9. Stale documentation — docs/FAQ.en.md:74,92 and docs/FAQ.es.md:91 still document the command format without [community]. All 10 YAML files were updated but the FAQs were missed.
10. Misleading name — communityName also accepts @handle and numeric group ids; the helper correctly calls it identifier. Suggest unifying on communityIdentifier in validations.ts and commands.ts.
11. Two queries where the old code advertised one — communityHelper.ts:95-102 issues two sequential findOne calls, and the comment removed from commands.ts read literally "Optimized community lookup - single database query instead of multiple". Also, the group match doesn't filter on public, so a private community whose group collides with a public community's name wins resolution. Edge case, but the precedence is undocumented.
12. Test style inconsistencies — both new files use require() + export {} while the rest of the suite (e.g. tests/bot/validation.spec.ts:1-17) uses ESM import. Also: createOrder.firstCall.args[3] is a magic index (extract a constant or destructure), the user object is shared and mutable across all 7 tests, and the docstring at orders-commands.spec.ts:554-561 references "the previous private-vs-group routing bug" — intra-PR history that means nothing to a future reader.
Verdict
Blocking: #1 (group spam and undeleted messages — an observable behavior regression) and #2 (COLLSCAN on the order-creation hot path, trivial to fix and it removes the regex surface as a bonus).
#3 and #4 need an explicit maintainer decision before merge: one can silently publish to the wrong community, the other changes moderation semantics in groups and contradicts both commit 1 and the locale text.
Everything else is polish.
About point 2: About test coverage it will be handled in the next commits |
|
Continued on #881 |
Add optional [community] param to /buy and /sell
Closes #877
What
Adds an optional trailing
[community]parameter to the/buyand/sellcommands so a user can publish an order to a specific community without having to switch their default community or copy and paste the order./sell (sats amount) (fiat amount) (fiat code) (payment method) [premium/discount] [community]
/buy (sats amount) (fiat amount) (fiat code) (payment method) [premium/discount] [community]
How it works
0if you don't want any). Positional parsing keeps numeric community identifiers (e.g. a Telegram group id) from being mistaken for a price margin, and makes any leftover non-numeric token fail the price-margin check instead of being silently swallowed as a community.groupfirst (the@handle/ group id, the same identifier used by/setcomm), matched case-insensitively (the group is stored lowercase and unique, so it stays unambiguous).name(the display name), matched exactly (case-sensitive) and restricted to public communities. The name only has a case-sensitive unique index, so an exact match stays unambiguous (Fooandfoocan no longer be confused), and private communities can only be reached through their canonical group id — matching pre-existing/setcommbehavior.community_not_foundmessage.Notes / scope
"bank transfer"), same as before. Because the fifth argument is now always parsed as the price margin, an unquoted multi-word payment method fails closed:/sell 0 2 USD bank transferreadsbankas the payment method andtransferas the price margin, which is rejected as a non-numeric margin instead of silently publishing a truncated order.to two channels introduces message-sync and order-identity complexity, so a usy runs the command again with the community argument. Open to aligning on this
before closing Add an optional [community] param to /buy and /sell commands #877.
Locales
sell_correct_format/buy_correct_formatin all 10 languages to[premium/discount], including a note that the premium/discount must beprovided first, respecting MarkdownV2 escaping.
Tests
/selland
/buy.getCommunityByIdentifierthat assert the final `cocanonical group id resolves (case-insensitive), display name is matched exactlyand only among public communities, and a private community is not reachable bytsc,lint,formatand the test suite pass (the one remaining failure iissue #867test).Summary by CodeRabbit
/buyand/sellnow accept an optional final community name to publish the order to a specific community, while still supporting the existing optional price margin.[community]argument.