✨ server: automate firewall allow after kyc approval - #1225
Conversation
🦋 Changeset detectedLatest commit: c34c470 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
WalkthroughThe server adds allow, poke, and credit queues with supervised workers. Persona and activity hooks enqueue work. Card creation queues credit processing. Runtime wiring, tests, translations, and release metadata are updated. ChangesQueue worker orchestration
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to The PR adds asynchronous firewall and credit-processing behavior, but the current implementation can silently skip distinct processing jobs, permanently lose credit work after persistence, duplicate user notifications during retries, and block user creation for malformed credentials. These correctness and availability risks make the PR unsafe to merge until addressed. Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The PR also adds credit queue processing, changes card creation, removes synchronous auto-credit logic, and updates related translations. These changes are not directly required by issue ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 53a3d3bf-6041-4b62-b601-afe847ac3bcb
📒 Files selected for processing (37)
.changeset/afraid-mangos-follow.md.changeset/blue-bottles-wave.md.changeset/bumpy-regions-read.md.changeset/free-lamps-pump.md.changeset/proud-tools-sneeze.md.changeset/tender-foxes-feel.mdinfra/utils/modules.tsserver/api/card.tsserver/api/index.tsserver/hooks/activity.tsserver/hooks/persona.tsserver/i18n/es.jsonserver/i18n/pt.jsonserver/index.tsserver/test/api/api.test.tsserver/test/api/card.test.tsserver/test/e2e.tsserver/test/hooks/activity.test.tsserver/test/hooks/persona.test.tsserver/test/mocks/deployments.tsserver/test/workers/allow.test.tsserver/test/workers/bin.test.tsserver/test/workers/credit.test.tsserver/test/workers/poke.test.tsserver/utils/panda.tsserver/workers/allow/bin.tsserver/workers/allow/job.tsserver/workers/allow/queue.tsserver/workers/allow/worker.tsserver/workers/credit/bin.tsserver/workers/credit/job.tsserver/workers/credit/queue.tsserver/workers/credit/worker.tsserver/workers/poke/bin.tsserver/workers/poke/job.tsserver/workers/poke/queue.tsserver/workers/poke/worker.ts
| await database.insert(cards).values([{ id: card.id, credentialId, lastFour: card.last4, productId }]); | ||
| await credit.enqueue(account).catch((error: unknown) => | ||
| captureException(error, { | ||
| level: "error", | ||
| tags: { queue: creditName, job: creditName }, | ||
| extra: { account }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make credit publication durable.
If credit.enqueue rejects, this handler only captures the error and returns card creation success. The card is already persisted. A later request returns "already created" at Line 601 and does not publish another job. An eligible account can then remain in debit mode after a transient Redis outage.
Persist an outbox record in the same transaction as the card insert. Dispatch and retry that record until the credit queue accepts it. Update the queue-failure test to verify eventual dispatch.
| await Promise.all( | ||
| [...pokes].map(([account, { assets, factory, publicKey, source }]) => | ||
| poke.enqueue({ | ||
| account, | ||
| assets: [...assets], | ||
| chainId: chain.id, | ||
| factory, | ||
| origin: "activity", | ||
| publicKey: bytesToHex(publicKey), | ||
| source, | ||
| }), | ||
| ), | ||
| ) | ||
| .then((results) => { | ||
| getActiveSpan()?.setStatus( | ||
| results.every((result) => result.status === "fulfilled") | ||
| ? { code: SPAN_STATUS_OK } | ||
| : { code: SPAN_STATUS_ERROR, message: "activity_failed" }, | ||
| ); | ||
| }) | ||
| .catch((error: unknown) => captureException(error)); | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Awaiting the enqueue makes webhook retries resend notifications.
A rejected poke.enqueue now returns 500, so Alchemy retries the webhook. The poke job dedupes through its job id, but the push notification sent earlier in the handler does not. Each retry delivers another "Funds received" message for the same transfer. Pass a deterministic idempotencyKey to onesignal.sendPushNotification (for example, the transfer hash plus asset) so retries do not duplicate user notifications.
| async function enqueueAllow(current: NonNullable<typeof credential>) { | ||
| if (account.success && firewallAddress) | ||
| await allow.enqueue({ | ||
| account: account.output, | ||
| chainId: chain.id, | ||
| factory: parse(Address, current.factory), | ||
| publicKey: bytesToHex(current.publicKey), | ||
| source: current.source, | ||
| }); | ||
| } | ||
| if (credential.pandaId) { | ||
| await enqueueAllow(credential); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate factory the same way as account.
account uses safeParse and the code skips the enqueue when parsing fails. factory uses parse, which throws. A credential row with a malformed factory therefore returns 500, blocks panda.createUser at Line 447, and makes Persona retry the webhook indefinitely. Use safeParse for factory and report the bad row instead.
🛠️ Proposed change
async function enqueueAllow(current: NonNullable<typeof credential>) {
- if (account.success && firewallAddress)
- await allow.enqueue({
- account: account.output,
- chainId: chain.id,
- factory: parse(Address, current.factory),
- publicKey: bytesToHex(current.publicKey),
- source: current.source,
- });
+ const factory = safeParse(Address, current.factory);
+ if (!account.success || !firewallAddress) return;
+ if (!factory.success) {
+ captureException(new Error("invalid factory address"), { level: "error", extra: { referenceId } });
+ return;
+ }
+ await allow.enqueue({
+ account: account.output,
+ chainId: chain.id,
+ factory: factory.output,
+ publicKey: bytesToHex(current.publicKey),
+ source: current.source,
+ });
}📝 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.
| async function enqueueAllow(current: NonNullable<typeof credential>) { | |
| if (account.success && firewallAddress) | |
| await allow.enqueue({ | |
| account: account.output, | |
| chainId: chain.id, | |
| factory: parse(Address, current.factory), | |
| publicKey: bytesToHex(current.publicKey), | |
| source: current.source, | |
| }); | |
| } | |
| if (credential.pandaId) { | |
| await enqueueAllow(credential); | |
| async function enqueueAllow(current: NonNullable<typeof credential>) { | |
| const factory = safeParse(Address, current.factory); | |
| if (!account.success || !firewallAddress) return; | |
| if (!factory.success) { | |
| captureException(new Error("invalid factory address"), { level: "error", extra: { referenceId } }); | |
| return; | |
| } | |
| await allow.enqueue({ | |
| account: account.output, | |
| chainId: chain.id, | |
| factory: factory.output, | |
| publicKey: bytesToHex(current.publicKey), | |
| source: current.source, | |
| }); | |
| } | |
| if (credential.pandaId) { | |
| await enqueueAllow(credential); |
| it("fails before panda creation when allow cannot be queued", async () => { | ||
| const error = new Error("redis unavailable"); | ||
| const errorConsole = vi.spyOn(console, "error").mockImplementation(() => undefined); | ||
| allow.enqueue.mockRejectedValueOnce(error); | ||
|
|
||
| const response = await appClient.index.$post({ | ||
| header: { | ||
| "persona-signature": "t=1733865120,v1=debbacfe1b0c5f8797a1d68e8428fba435aa4ca3b5d9a328c3c96ee4d04d84df", | ||
| }, | ||
| json: { | ||
| ...validPayload, | ||
| data: { | ||
| ...validPayload.data, | ||
| attributes: { | ||
| ...validPayload.data.attributes, | ||
| payload: { | ||
| ...validPayload.data.attributes.payload, | ||
| included: [...validPayload.data.attributes.payload.included], | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(response.status).toBe(500); | ||
| expect(errorConsole).toHaveBeenCalledWith(error); | ||
| expect(allow.enqueue).toHaveBeenCalledOnce(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reuse the postInquiry helper.
Lines 599-616 rebuild the same request that postInquiry builds at Line 2004, including the identical signature header. Call the helper to remove the duplicate payload.
♻️ Proposed refactor
- const response = await appClient.index.$post({
- header: {
- "persona-signature": "t=1733865120,v1=debbacfe1b0c5f8797a1d68e8428fba435aa4ca3b5d9a328c3c96ee4d04d84df",
- },
- json: {
- ...validPayload,
- data: {
- ...validPayload.data,
- attributes: {
- ...validPayload.data.attributes,
- payload: {
- ...validPayload.data.attributes.payload,
- included: [...validPayload.data.attributes.payload.included],
- },
- },
- },
- },
- });
+ const response = await postInquiry();📝 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.
| it("fails before panda creation when allow cannot be queued", async () => { | |
| const error = new Error("redis unavailable"); | |
| const errorConsole = vi.spyOn(console, "error").mockImplementation(() => undefined); | |
| allow.enqueue.mockRejectedValueOnce(error); | |
| const response = await appClient.index.$post({ | |
| header: { | |
| "persona-signature": "t=1733865120,v1=debbacfe1b0c5f8797a1d68e8428fba435aa4ca3b5d9a328c3c96ee4d04d84df", | |
| }, | |
| json: { | |
| ...validPayload, | |
| data: { | |
| ...validPayload.data, | |
| attributes: { | |
| ...validPayload.data.attributes, | |
| payload: { | |
| ...validPayload.data.attributes.payload, | |
| included: [...validPayload.data.attributes.payload.included], | |
| }, | |
| }, | |
| }, | |
| }, | |
| }); | |
| expect(response.status).toBe(500); | |
| expect(errorConsole).toHaveBeenCalledWith(error); | |
| expect(allow.enqueue).toHaveBeenCalledOnce(); | |
| it("fails before panda creation when allow cannot be queued", async () => { | |
| const error = new Error("redis unavailable"); | |
| const errorConsole = vi.spyOn(console, "error").mockImplementation(() => undefined); | |
| allow.enqueue.mockRejectedValueOnce(error); | |
| const response = await postInquiry(); | |
| expect(response.status).toBe(500); | |
| expect(errorConsole).toHaveBeenCalledWith(error); | |
| expect(allow.enqueue).toHaveBeenCalledOnce(); |
| if (job.data.origin === "activity") { | ||
| await credit.enqueue(job.data.account, `poke-${job.id}`); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Avoid a shared credit job id when job.id is undefined.
BullMQ types job.id as string | undefined. If it is undefined, the credit job id becomes the literal "poke-undefined", which every account shares. The credit queue then treats unrelated accounts as duplicates and drops their credit processing. Fall back to the account.
🛠️ Proposed change
- await credit.enqueue(job.data.account, `poke-${job.id}`);
+ await credit.enqueue(job.data.account, `poke-${job.id ?? job.data.account}`);📝 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.
| if (job.data.origin === "activity") { | |
| await credit.enqueue(job.data.account, `poke-${job.id}`); | |
| } | |
| if (job.data.origin === "activity") { | |
| await credit.enqueue(job.data.account, `poke-${job.id ?? job.data.account}`); | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1225 +/- ##
==========================================
- Coverage 73.48% 73.24% -0.24%
==========================================
Files 276 288 +12
Lines 13454 13397 -57
Branches 4732 4610 -122
==========================================
- Hits 9887 9813 -74
- Misses 3231 3243 +12
- Partials 336 341 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 597de4d62d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| publicKey, | ||
| source, | ||
| }: Omit<Job, "sentryBaggage" | "sentryTrace">) { | ||
| await instance.enqueue({ account, assets, chainId, factory, publicKey, source }, account); |
There was a problem hiding this comment.
If the allow worker exhausts its retries (for example during an RPC/KMS outage), the failed job is kept by the shared queue defaults, and this stable account job id means any later Persona retry for the same approved account is treated by BullMQ as a duplicate instead of adding fresh work. The webhook will then continue as though allow was queued, but the firewall allow transaction will never run until the failed job is manually removed or expires, leaving KYC-approved accounts blocked from the firewall flow.
Useful? React with 👍 / 👎.
| }: Omit<Job, "sentryBaggage" | "sentryTrace">) { | ||
| await instance.enqueue( | ||
| { account, assets, chainId, factory, origin, publicKey, source }, | ||
| [account, ...(assets ?? [])].join("-"), |
There was a problem hiding this comment.
When the same account receives the same asset on two supported Alchemy networks while the first poke job is still present, both publishes use the same custom job id because this id only includes account and assets. BullMQ deduplicates custom job ids, so the second chain's deploy/poke work is silently dropped; include chainId in the id to keep cross-chain activity independent.
Useful? React with 👍 / 👎.
| await instance.enqueue( | ||
| { account, assets, chainId, factory, origin, publicKey, source }, | ||
| [account, ...(assets ?? [])].join("-"), | ||
| ); |
There was a problem hiding this comment.
The new server/test/workers/poke.test.ts test expects startSpan to be called with name: "account poke", but this call leaves the shared queue helper's spanName argument unset, so it emits the default "poke" span instead. As written, the added worker test fails until this enqueue call passes the intended span name or the assertion is updated.
Useful? React with 👍 / 👎.
| const wallet = createWallet(poker, chain); | ||
| const isDeployed = !!(await wallet.getCode({ address: job.data.account })); | ||
| span.setAttribute("exa.new", !isDeployed); | ||
| if (!isDeployed) { |
There was a problem hiding this comment.
When a Persona approval enqueues an allow job, it reaches this worker with origin: "allow" and no assets; if that KYC-approved account has not received funds, this block still deploys the account before the balance scan below can determine that poked remains false. That makes every unfunded KYC approval spend the poker signer’s gas and emits an AccountFunded track event even though no funds were present; check balances first for allow jobs and only deploy when there is something to poke.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cdc276fc-7a05-4655-bace-fdabd3bdfb4f
📒 Files selected for processing (3)
server/test/hooks/persona.test.tsserver/test/mocks/deployments.tsserver/test/workers/poke.test.ts
| async function jobFinished( | ||
| current: Parameters<ReturnType<typeof createPoke>["enqueue"]>[0], | ||
| options?: JobsOptions, | ||
| trace?: Pick<Poke, "sentryBaggage" | "sentryTrace">, | ||
| ) { | ||
| const id = [current.account, ...(current.assets ?? [])].join("-"); | ||
| const job = await queue.add( | ||
| "poke", | ||
| { ...current, ...trace }, | ||
| { attempts: 1, jobId: id, removeOnComplete: true, removeOnFail: true, ...options }, | ||
| ); | ||
| await job.waitUntilFinished(events).catch(async (error: unknown) => { | ||
| await vi.waitUntil(() => vi.mocked(captureException).mock.calls.length > 0); | ||
| throw error; | ||
| }); | ||
| return job; | ||
| } | ||
|
|
||
| async function spyScopeSetUser() { | ||
| const { withScope: realWithScope } = await vi.importActual<typeof sentry>("@sentry/node"); | ||
| const setUser = vi.fn(); | ||
| vi.mocked(withScope).mockImplementation((_scopeOrCallback, _callback?) => | ||
| realWithScope((scope) => { | ||
| const originalSetUser = scope.setUser.bind(scope); | ||
| scope.setUser = (...args: Parameters<typeof scope.setUser>) => { | ||
| setUser(...args); | ||
| return originalSetUser(...args); | ||
| }; | ||
| return ((_callback ?? _scopeOrCallback) as NonNullable<typeof _callback>)(scope); | ||
| }), | ||
| ); | ||
| return setUser; | ||
| } | ||
|
|
||
| async function spySpanSetAttribute() { | ||
| const { startSpan: realStartSpan } = await vi.importActual<typeof sentry>("@sentry/node"); | ||
| const setAttribute = vi.fn(); | ||
| vi.mocked(startSpan).mockImplementation(((options, callback) => | ||
| realStartSpan(options, (span) => { | ||
| const originalSetAttribute = span.setAttribute.bind(span); | ||
| span.setAttribute = (...args: Parameters<typeof span.setAttribute>) => { | ||
| setAttribute(...args); | ||
| return originalSetAttribute(...args); | ||
| }; | ||
| return callback(span); | ||
| })) as typeof startSpan); | ||
| return setAttribute; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the standalone helpers to the bottom of the file.
jobFinished, spyScopeSetUser, and spySpanSetAttribute are supporting details. The coding guidelines place standalone function declarations at the bottom, next to internal constants and types. server/test/hooks/persona.test.ts already follows this pattern with postInquiry at Line 2005. Function declarations are hoisted, so the move is safe.
As per coding guidelines: "place the default export (the thing the file exists for) at the top. standalone function declarations are supporting details and belong at the bottom alongside internal constants and types."
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6721df821e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (job.data.origin === "activity") { | ||
| await credit.enqueue(job.data.account, `poke-${job.id}`); | ||
| } |
There was a problem hiding this comment.
When an already-created KYC credential has an active debit-mode card and funds were waiting behind the firewall, the allow job can successfully poke those balances, but this condition excludes origin: "allow" from enqueueing the credit worker. The original inbound activity may already have exhausted its retries before permission was granted, so no later event is guaranteed to evaluate automatic credit and the eligible card remains in debit mode until another deposit or manual action.
Useful? React with 👍 / 👎.
| .catch((error: unknown) => captureException(error, { level: "error" })); | ||
| } | ||
| if (job.data.origin === "activity") { | ||
| await credit.enqueue(job.data.account, `poke-${job.id}`); |
There was a problem hiding this comment.
When a later activity creates a new poke job for the same account and assets while the previous credit job is still among the 100 retained completions, the poke queue reuses the same job.id, making poke-${job.id} collide with the completed credit job. BullMQ then returns that existing job instead of running another credit evaluation, so changes such as removing a USDC deposit and receiving more of the same collateral can leave automatic credit disabled until the retained job is evicted.
Useful? React with 👍 / 👎.
| if (job.data.origin === "activity") { | ||
| await job.updateData({ | ||
| ...job.data, |
There was a problem hiding this comment.
If this Redis update fails, or the worker exits after the on-chain transaction succeeds but before this checkpoint is persisted, the retry still contains the asset that was already poked. Its wallet balance is then zero because the funds are in the market, so the retry classifies it as pending, repeatedly throws NoBalance(), and never reaches the credit enqueue; a transient checkpoint failure can therefore turn a successful deposit into a terminal job without automatic credit processing.
Useful? React with 👍 / 👎.
| workers: { | ||
| allow: { signers: ["allower"] }, | ||
| credit: { secrets: ["onesignal-api-key", "postgres-url"] }, | ||
| poke: { secrets: ["onesignal-api-key", "segment-write-key"], signers: ["poker"] }, |
There was a problem hiding this comment.
In an infra-deployed worker this creates and loads a distinct poker KMS account, but the deployed plugin grants KEEPER_ROLE only to acct("keeper") in contracts/script/ExaPlugin.s.sol, while poke and pokeETH are explicitly mapped to keeper runtime validation in contracts/src/ExaPlugin.sol; a repo-wide search finds no role grant for the new poker address. Consequently every main-chain poke sent by this worker reverts Unauthorized and exhausts its retries, so the worker must reuse the authorized keeper or provision an on-chain grant for the KMS address.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d189bff56
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }: Omit<Job, "sentryBaggage" | "sentryTrace">) { | ||
| await instance.enqueue( | ||
| { account, assets, chainId, factory, origin, publicKey, source }, | ||
| [account, ...(assets ?? [])].join("-"), |
There was a problem hiding this comment.
When a poke job exhausts its retries, the shared queue retains it for up to seven days (server/workers/queue.ts:22). Because this ID is deterministic for an account and its assets, a later Alchemy event for the same account and asset is treated by BullMQ as the existing failed job instead of starting new work, so deposits made after a temporary outage remain unpoked until cleanup; remove failed poke jobs or use a unique activity-execution ID.
Useful? React with 👍 / 👎.
| pending.push(asset); | ||
| continue; | ||
| } | ||
| const receipt = await wallet.exaSend( |
There was a problem hiding this comment.
When one activity contains multiple supported assets and an earlier asset's poke persistently reverts, this sequential await aborts every attempt at that asset. Since the failed asset remains ahead of the untouched assets in the checkpointed job data, later valid assets are never visited and remain idle after the job exhausts its retries; process the assets independently or split the remaining assets into separate work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fc82aa002
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .insert(cards) | ||
| .values([{ id: card.id, credentialId, lastFour: card.last4, mode, productId }]); | ||
| await database.insert(cards).values([{ id: card.id, credentialId, lastFour: card.last4, productId }]); | ||
| await credit.enqueue(account).catch((error: unknown) => |
There was a problem hiding this comment.
When an already-funded account creates its card while Redis or BullMQ is temporarily unavailable, this catch returns success after storing the card in debit mode but discards the only credit evaluation scheduled by card creation. The earlier deposit's activity job may have completed before the card existed, and no later event is guaranteed to enqueue another evaluation, so an otherwise eligible card can remain in debit mode indefinitely; persist or retry this work without failing card issuance.
Useful? React with 👍 / 👎.
| captureException(error, { level: "error", fingerprint: revertFingerprint(error) }); | ||
| }); | ||
| throw error; | ||
| await Promise.all( |
There was a problem hiding this comment.
When poke.enqueue rejects after the notification promise above has started, this awaited publication returns a 500 even though the user may already have received the funds notification. A normal webhook redelivery then executes the notification path again, so a transient Redis failure can send duplicate push notifications; publish the durable job before sending the notification or otherwise deduplicate that side effect. .agents/rules/server.mdL75-L76
Useful? React with 👍 / 👎.
| const card = credential?.cards[0]; | ||
| span.setAttribute("exa.card", card?.id); | ||
| if (card?.mode === 0) { | ||
| await database.update(cards).set({ mode: 1 }).where(eq(cards.id, card.id)); |
There was a problem hiding this comment.
Because card creation now stores mode 0 and delegates activation to this asynchronous worker, a concurrent card PATCH can change the mode or delete the card after this worker reads card.mode but before this unconditional update runs. The worker then overwrites the user's selected installment mode—or updates a deleted card—and sends an incorrect activation notification; include the observed mode and active status in the update predicate and notify only when a row was actually changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f7457be78
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async enqueue(account: Job["account"], jobId: string = account) { | ||
| await instance.enqueue({ account }, jobId); |
There was a problem hiding this comment.
When a user replaces a deleted card within seven days after the previous card's automatic-credit job exhausted its retries, this default ID reuses the account address while the failed job is still retained by server/workers/queue.ts. BullMQ therefore treats the replacement card's enqueue as a duplicate, so the new card never gets its collateral evaluated and remains in debit mode until another activity happens to schedule a differently named credit job; remove failed account-scoped jobs or make each card-creation check unique.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9e6e7895
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| common: ["redis-url", "sentry-dsn"], | ||
| crema: ["redis-address", "redis-password", "redis-username"], | ||
| workers: { | ||
| allow: { signers: ["allower"] }, |
There was a problem hiding this comment.
In an infra deployment this creates a new allower KMS account, and server/workers/allow/worker.ts uses that EOA to call Firewall.allow directly, but a repo-wide search finds no on-chain role grant or other provisioning for its address. Consequently Persona approvals enqueue transactions from an unauthorized signer, so allow jobs exhaust their retries and approved accounts remain blocked; reuse the already-authorized account or add an explicit firewall grant during deployment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd1003bae7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }: Omit<Job, "sentryBaggage" | "sentryTrace">) { | ||
| await instance.enqueue( | ||
| { account, assets, chainId, factory, origin, publicKey, source }, | ||
| [account, ...(assets ?? [])].join("-"), |
There was a problem hiding this comment.
When a second deposit of the same asset reaches the same account while the first poke job still exists, this deterministic ID makes BullMQ return the existing job without merging the new activity. If the first worker has already completed its on-chain poke but has not yet finished the job, the second deposit remains in the wallet and its webhook cannot schedule another poke; include an activity-specific identifier or explicitly merge/requeue duplicate activity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
server/workers/poke/queue.ts (1)
19-22: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
originto the job ID and normalize asset order.The job ID is
chainId-account-assets. It omitsorigin. BullMQ ignores anaddwith an existingjobIdinstead of replacing the payload. Two producers can therefore collide:
server/hooks/activity.tsLine 189 enqueuesorigin: "activity".server/workers/allow/worker.tsenqueuesorigin: "allow"with the sameaccount,chainId, andassets.When both jobs share the same account, chain, and asset list, the second
enqueueis dropped. The dropped job's origin-specific behavior inserver/workers/poke/worker.tsis then skipped: the"allow"push notification, or the"activity"credit enqueue and pending-asset retry.Asset order is also not normalized.
[weth, usdc]and[usdc, weth]produce different IDs for the same account, so two concurrent jobs poke the same account.Sort the assets and include
originin the ID.🐛 Proposed fix
await instance.enqueue( { account, assets, chainId, factory, origin, publicKey, source }, - [chainId, account, ...(assets ?? [])].join("-"), + [chainId, account, origin, ...[...(assets ?? [])].sort()].join("-"), );Note:
server/test/workers/poke.test.tsLine 79 and Lines 147, 180, 201, 500, 568, 613 build the expected ID with the same formula. Update them together.server/hooks/activity.ts (1)
176-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an idempotency key to the push notification.
Line 187 awaits
poke.enqueue. A rejection returns 500, so Alchemy retries the webhook delivery. The poke job deduplicates through its job ID, but the notification sent at Line 177 does not. Each retry delivers another "Funds received" message for the same transfer.Pass a deterministic
idempotencyKeytoonesignal.sendPushNotification.server/utils/onesignal.tsalready forwards it to OneSignal.🛡️ Proposed fix
- for (const { toAddress: account, rawContract, value, asset: assetSymbol } of transfers) { + for (const { toAddress: account, hash, rawContract, value, asset: assetSymbol } of transfers) { if (!accounts[account]) continue; if (chain.id === exaChain.id && rawContract?.address && markets.has(rawContract.address)) continue; const asset = rawContract?.address ?? ETH; const underlying = asset === ETH ? WETH : asset; const notification = { userId: account, + idempotencyKey: `${hash}-${asset}`, headings: t("Funds received"),server/test/workers/poke.test.ts (1)
74-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the standalone helpers to the bottom of the file.
jobFinished,spyScopeSetUser, andspySpanSetAttributeare supporting details.createRequest,getMarket, andmintalready sit at the bottom of this file. Move these three next to them. Function declarations are hoisted, so the move is safe.As per coding guidelines: "place the default export (the thing the file exists for) at the top. standalone function declarations are supporting details and belong at the bottom alongside internal constants and types."
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c335e9ea-3755-4404-b17f-7937b8be58c4
📒 Files selected for processing (8)
infra/utils/modules.tsserver/hooks/activity.tsserver/index.tsserver/test/e2e.tsserver/test/hooks/activity.test.tsserver/test/workers/bin.test.tsserver/test/workers/poke.test.tsserver/workers/poke/queue.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
co-authored-by: Miguel Diaz <github.com.hf06j@slmail.me>
co-authored-by: Miguel Diaz <github.com.hf06j@slmail.me>
1 participant
closes #643
Summary by CodeRabbit