Conversation
🦋 Changeset detectedLatest commit: 2ffe1cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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:
WalkthroughAdds GCP KMS support and an allower client, centralizes per-account processing in the activity hook to ensure deployment → poke → autoCredit, introduces pokeAccountAssets utility and firewall/allower wiring in persona hook, and adds tests and runtime/config changes for GCP and poking flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Activity as "Activity Hook"
participant Factory as "Account Factory"
participant Keeper as "Keeper/Wallet"
participant Poke as "pokeAccountAssets"
participant Credit as "autoCredit"
participant Notify as "Push Service"
Activity->>Factory: ensureDeployed(account)
Factory->>Keeper: deploy/create account if needed
Keeper-->>Factory: deployment result
Factory-->>Activity: deployed
Activity->>Poke: pokeAccountAssets(account, options)
Poke->>Keeper: read balances + market mappings
Keeper-->>Poke: balances
Poke->>Keeper: exaSend poke (ETH/ERC-20) (batched)
Keeper-->>Poke: poke results
Activity->>Credit: autoCredit(account)
Credit->>Keeper: resolve credentials / select card / enable credit mode
Keeper-->>Credit: credit result
Poke->>Notify: send notification (if applicable)
Notify-->>Activity: notified
sequenceDiagram
participant Persona as "Persona Hook"
participant Allower as "Allower / Firewall"
participant Poke as "pokeAccountAssets"
participant Keeper as "Keeper/Wallet"
Persona->>Allower: get client & authorize(account, token)
Allower-->>Persona: success / error
alt account parsed successfully
Persona->>Poke: pokeAccountAssets(account)
Poke->>Keeper: read balances & poke
Keeper-->>Poke: results
Poke-->>Persona: done
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @aguxez, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the server-side logic for interacting with user accounts, specifically concerning the 'poking' of assets to potentially activate yield or other on-chain functionalities. The previous implementation, which attempted to poke accounts upon any fund receipt, has been replaced with a more targeted approach. Now, accounts will be automatically 'poked' for their relevant assets immediately following a successful Know Your Customer (KYC) verification through the Persona integration. This change streamlines the user onboarding experience by ensuring that accounts are properly initialized for asset interaction right after identity verification, while also simplifying the general activity tracking mechanism. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the account poking logic, moving it from the activity hook to the persona hook to be triggered after KYC completion. While adding a poke after KYC is beneficial, removing this logic entirely from the activity hook is a critical regression, as it prevents subsequent deposits from automatically earning yield. Additionally, the new implementation in persona.ts lacks the robustness of the original, specifically the retry mechanism.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #666 +/- ##
===========================================
+ Coverage 68.15% 79.18% +11.03%
===========================================
Files 207 57 -150
Lines 6949 2777 -4172
Branches 2167 636 -1531
===========================================
- Hits 4736 2199 -2537
+ Misses 2021 362 -1659
- Partials 192 216 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/hooks/persona.ts (1)
320-345: Redundant address parsing - parse once and reuse.The same
credential.accountis parsed twice with different methods:
- Line 322:
v.parse(Address, credential.account)(throws on failure)- Line 327:
safeParse(Address, credential.account)(returns result)Consider consolidating to parse once before both operations.
♻️ Proposed refactor to eliminate redundant parsing
- Promise.resolve() - .then(async () => { - const accountAddress = v.parse(Address, credential.account); - await pokeAccountAssets(accountAddress); - }) - .catch((error: unknown) => captureException(error)); - - const accountParsed = safeParse(Address, credential.account); - if (accountParsed.success) { + const accountParsed = safeParse(Address, credential.account); + if (accountParsed.success) { + const accountAddress = accountParsed.output; + pokeAccountAssets(accountAddress).catch((error: unknown) => captureException(error)); + addCapita({ birthdate: fields.birthdate.value, document: fields.identificationNumber.value, firstName: fields.nameFirst.value, lastName: fields.nameLast.value, email: fields.emailAddress.value, phone: fields.phoneNumber?.value ?? "", - internalId: deriveAssociateId(accountParsed.output), + internalId: deriveAssociateId(accountAddress), product: "travel insurance", }).catch((error: unknown) => { captureException(error, { extra: { pandaId: id, referenceId } }); }); } else { captureException(new Error("invalid account address"), { extra: { pandaId: id, referenceId, account: credential.account }, }); }
🤖 Fix all issues with AI agents
In `@server/hooks/activity.ts`:
- Around line 105-132: The code repeatedly calls v.parse(Address, account)
inside the startSpan async callback; parse the account once into a local
variable (e.g., parsedAccount) at the top of that callback and reuse it for all
uses (scope.setUser, scope.setTag, publicClient.getCode, keeper.exaSend
args/track, and autoCredit) to avoid duplicate parsing and improve clarity;
update all references of v.parse(Address, account) to use the new parsed
variable and ensure its type is the parsed Address type expected by
publicClient, keeper.exaSend, track, and autoCredit.
In `@server/hooks/persona.ts`:
- Around line 46-110: The duplicate concatenation of ABIs inside
pokeAccountAssets leads to repeated array construction; extract the combined ABI
into a single constant (e.g., const combinedAccountAbi = [...exaPluginAbi,
...upgradeableModularAccountAbi, ...auditorAbi, ...marketAbi]) and replace both
inline concatenations in the keeper.exaSend calls with combinedAccountAbi so
pokeAccountAssets uses the shared constant instead of building the same array
twice.
In `@server/test/hooks/persona.test.ts`:
- Around line 555-560: The current fragile fixed delay (setTimeout 100ms) before
asserting expect(exaSendSpy).not.toHaveBeenCalled() can flake in slow CI;
replace it with a deterministic wait using vitest's vi.waitFor (or equivalent
test-wait helper) to repeatedly check that exaSendSpy has not been called with a
reasonable timeout/interval, or increase the delay with a clear comment if you
must keep the sleep. Locate the assertion in persona.test.ts around the
setTimeout block and update the logic that waits prior to asserting on
exaSendSpy (the spy variable is exaSendSpy) to use vi.waitFor with an early-exit
negative assertion or a longer buffered timeout to make the test robust.
aguxez
force-pushed
the
kyc-poke
branch
2 times, most recently
from
6f7b98a to
9168653
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/hooks/activity.ts`:
- Around line 106-136: The call inside keeper.exaSend uses v.parse(Address,
factory) redundantly—factory was already parsed to Address earlier—so remove the
extra parse and pass the existing parsed factory value (e.g., factory or
parsedFactory as used where it was parsed) directly to keeper.exaSend's address
field; update the keeper.exaSend invocation in the account activity span (inside
startSpan / withScope) to use the parsed factory variable and ensure types still
align.
In `@server/hooks/persona.ts`:
- Around line 48-70: In pokeAccountAssets wrap the initial
publicClient.readContract call that fetches assets from exaPreviewerAddress /
functionName "assets" with the existing withRetry helper (same retry/backoff
config you use for the poke operations) so transient RPC failures are retried;
keep the subsequent mapping to new Map<Address,Address>(...) and the parsing
with v.parse unchanged, but invoke withRetry around the readContract Promise
(referencing publicClient.readContract, exaPreviewerAddress, exaPreviewerAbi,
and withRetry) to make asset discovery resilient.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/hooks/activity.ts`:
- Around line 100-101: The notification string assembly currently uses `${value
? `${value} ` : ""}${assetSymbol} received` and can produce "undefined received"
when asset/assetSymbol is missing; update the construction in the block that
sets the `en` notification text to guard against missing asset by using a safe
fallback (e.g. use assetSymbol ?? "" or asset?.symbol ?? "" and if still empty
fall back to a generic word like "token" or omit the symbol) and trim or
conditionally include the trailing space so you never log "undefined received";
apply the same guard for any parallel localization keys if present.
In `@server/hooks/persona.ts`:
- Around line 65-79: The current Promise.all over ERC‑20 reads will reject on
any single read error and skip ETH & other balances; change to a best‑effort
collection by replacing the inner
Promise.all([...marketsByAsset.entries()].map(...)) with a Promise.allSettled
(or map that catches errors) so each publicClient.readContract call for ERC20
(using erc20Abi, publicClient.readContract, marketsByAsset, asset) is allowed to
fail individually and return a safe fallback (e.g., balance = 0 or null plus
asset/market) instead of throwing; keep the publicClient.getBalance call for ETH
(ethBalance) as-is and combine results so assetBalances contains entries for
every asset with error-safe balances.
In `@server/test/hooks/persona.test.ts`:
- Around line 262-266: The inline comment inside the afterEach block should be
lowercased for style compliance: locate the afterEach function (the cleanup hook
that calls database.update(credentials).set({ pandaId: null
}).where(eq(credentials.id, "persona-ref")) and vi.restoreAllMocks()) and change
the comment text "// Reset pandaId to null for next test" to use lowercase
(e.g., "// reset pandaId to null for next test").
♻️ Duplicate comments (1)
server/hooks/activity.ts (1)
106-137: Confirm poke gating bypandaIdmatches intended product behavior.With this guard, deposits made before KYC won’t be poked until KYC completes. If that’s not the desired behavior, move the call outside the
pandaIdcheck or document the change explicitly.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/hooks/persona.ts`:
- Around line 65-83: The current Promise.allSettled mapping loses asset/market
context and accesses result.reason unsafely; change to map over
[...marketsByAsset.entries()] with an async function that wraps each
publicClient.readContract call in a try/catch so each iteration always returns a
typed object { asset, market, balance } (on success use the returned balance, on
failure set balance to 0n and log the error), then await Promise.all over those
per-asset promises; update the code paths referencing marketsByAsset,
publicClient.readContract, and erc20Abi to use this per-asset try/catch pattern
to avoid unsafe result.reason access and preserve asset/market information.
In `@server/test/hooks/persona.test.ts`:
- Around line 555-556: Update inline comment text to use lowercase per style
guidelines: change the two comment lines that mention vi.waitFor and
pokeAccountAssets in persona.test.ts to start with lowercase letters (e.g., "use
vi.waitFor..." and "the wait is necessary...") so they comply with the project's
inline comment style; locate the comments near the test that references
vi.waitFor and pokeAccountAssets and adjust only the capitalization of those
comment sentences.
- Line 489: Change the inline comment "Should poke ETH and the other asset, but
skip WETH" to use lowercase initial letter for style compliance; locate that
comment in server/test/hooks/persona.test.ts (the inline test comment at the
marked diff) and update it to "should poke ETH and the other asset, but skip
WETH" (or fully lowercase asset names if your style prefers "eth" and "weth") so
it follows the project's lowercase-inline-comment guideline.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/hooks/persona.ts`:
- Around line 353-362: The code parses credential.account twice (via v.parse and
safeParse); parse it once into a single variable (e.g., accountParsed or
accountResult) before both uses and reuse that parsed Address when calling
deriveAssociateId and addCapita to avoid duplicated work and potential
inconsistencies; update references in the addCapita call to use the single
parsed output (from Address) and adjust error handling/guards to rely on that
one parse result (symbols: Address, safeParse, v.parse, deriveAssociateId,
addCapita, credential.account).
In `@server/test/hooks/persona.test.ts`:
- Line 489: The inline comment "should poke ETH and the other asset, but skip
WETH" should be converted to all lowercase to meet style guidelines; locate the
inline comment with that exact text in persona.test.ts (near the test case
containing that comment) and change it to "should poke eth and the other asset,
but skip weth".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/hooks/persona.ts`:
- Around line 49-59: The variable names `p` and `m` in the `marketsByAsset`
assignment are cryptic; update the callback and mapping parameter names to be
descriptive (e.g., rename `p` to `assetsList` or `assetsArray` and `m` to
`assetEntry` or `assetRecord`) inside the `withRetry(...).then(...)` chain so
the call to `publicClient.readContract({ address: exaPreviewerAddress,
functionName: "assets", abi: exaPreviewerAbi })` and the mapping using
`v.parse(Address, ... )` reads clearly (e.g., `.then((assetsArray) => new
Map<Address, Address>(assetsArray.map((assetEntry) => [v.parse(Address,
assetEntry.asset), v.parse(Address, assetEntry.market)])))`). Ensure only the
variable names change, preserving `marketsByAsset`, `withRetry`,
`publicClient.readContract`, `exaPreviewerAddress`, `exaPreviewerAbi`, and
`v.parse(Address, ...)`.
♻️ Duplicate comments (1)
server/test/hooks/persona.test.ts (1)
489-489: Lowercase inline comment for style compliance.✏️ Suggested tweak
- // should poke ETH and the other asset, but skip WETH + // should poke eth and the other asset, but skip wethAs per coding guidelines, ...
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/test/hooks/persona.test.ts (1)
705-722: Missing spy causes pipeline failure.The test asserts
expect(panda.createUser).not.toHaveBeenCalled()at line 721, butpanda.createUseris not spied on in this test. SinceafterEachcallsvi.restoreAllMocks(), spies from previous tests are not available.🐛 Proposed fix - add spy before assertion
it("handles manteca template and adds document", async () => { vi.spyOn(persona, "addDocument").mockResolvedValueOnce({ data: { id: "doc_manteca" } }); + vi.spyOn(panda, "createUser"); const response = await appClient.index.$post({ header: { "persona-signature": "t=1,v1=sha256" }, json: mantecaPayload, }); expect(response.status).toBe(200); await expect(response.json()).resolves.toStrictEqual({ code: "ok" }); expect(persona.addDocument).toHaveBeenCalledWith(referenceId, { id_class: { value: "dl" }, id_number: { value: "ID12345" }, id_issuing_country: { value: "AR" }, id_document_id: { value: "doc_gov_123" }, }); expect(panda.createUser).not.toHaveBeenCalled(); });
♻️ Duplicate comments (3)
server/hooks/persona.ts (2)
398-424: Consider consolidating address parsing.The account address is parsed twice: once with
v.parseat line 400 and again withsafeParseat line 405. Since both need a valid address, consider parsing once before both usages.♻️ Optional consolidation
- Promise.resolve() - .then(async () => { - const accountAddress = v.parse(Address, credential.account); - await pokeAccountAssets(accountAddress); - }) - .catch((error: unknown) => captureException(error)); - - const accountParsed = safeParse(Address, credential.account); - if (accountParsed.success) { + const accountParsed = safeParse(Address, credential.account); + if (accountParsed.success) { + pokeAccountAssets(accountParsed.output).catch((error: unknown) => captureException(error)); + addCapita({ birthdate: fields.birthdate.value, // ...rest of addCapita call internalId: deriveAssociateId(accountParsed.output), product: "travel insurance", }).catch((error: unknown) => { captureException(error, { level: "error", extra: { pandaId: id, referenceId } }); }); + } else { + captureException(new Error("invalid account address"), { + extra: { pandaId: id, referenceId, account: credential.account }, + level: "error", + }); }
50-60: Use descriptive names for asset mapping variables.The variable names
pandmare cryptic and violate coding guidelines which require full descriptive names.♻️ Suggested rename
- ).then((p) => new Map<Address, Address>(p.map((m) => [v.parse(Address, m.asset), v.parse(Address, m.market)]))); + ).then((assetMarkets) => + new Map<Address, Address>( + assetMarkets.map((entry) => [v.parse(Address, entry.asset), v.parse(Address, entry.market)]), + ), + );As per coding guidelines, avoid abbreviations or cryptic names.
server/hooks/activity.ts (1)
135-164: Verify race condition between pokeAccountAssets and autoCredit is acceptable.Both
pokeAccountAssetsandautoCreditare fire-and-forget and run concurrently. IfautoCreditreads on-chain state beforepokeAccountAssetscompletes its transactions, it may see stalefloatingDepositAssetsvalues and return incorrect results.If auto-credit should only activate after assets are deposited into markets, consider chaining them:
if (pandaId) { pokeAccountAssets(parsedAccount) .then(() => autoCredit(parsedAccount)) .then(async (auto) => { // handle auto credit result }) .catch((error: unknown) => captureException(error)); } else { autoCredit(parsedAccount).then(/* ... */).catch(/* ... */); }If the current concurrent behavior is intentional (auto-credit operates independently of poke), please confirm this is the expected design.
aguxez
force-pushed
the
kyc-poke
branch
2 times, most recently
from
a4f6ae3 to
77abbaf
Compare
cruzdanilo
force-pushed
the
main
branch
2 times, most recently
from
c7829dc to
5e7861d
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/hooks/persona.ts (1)
315-343: 🧹 Nitpick | 🔵 TrivialMerge the two
accountParsed.successbranches.Lines 316 and 336 both guard on
accountParsed.success. These could be a single block, reducing duplication and making the flow clearer.♻️ Proposed consolidation
const accountParsed = safeParse(Address, credential.account); if (accountParsed.success) { addCapita({ birthdate: fields.birthdate.value, document: fields.identificationNumber.value, firstName: fields.nameFirst.value, lastName: fields.nameLast.value, email: fields.emailAddress.value, phone: fields.phoneNumber?.value ?? "", internalId: deriveAssociateId(accountParsed.output), product: "travel insurance", }).catch((error: unknown) => { captureException(error, { level: "error", extra: { pandaId: id, referenceId } }); }); + pokeAccountAssets(accountParsed.output, { + notification: { + headings: { en: "Account assets updated" }, + contents: { en: "Your funds are ready to use" }, + }, + }).catch(captureException); } else { captureException(new Error("invalid account address"), { extra: { pandaId: id, referenceId, account: credential.account }, level: "error", }); } - - if (accountParsed.success) { - pokeAccountAssets(accountParsed.output, { - notification: { - headings: { en: "Account assets updated" }, - contents: { en: "Your funds are ready to use" }, - }, - }).catch(captureException); - }
🤖 Fix all issues with AI agents
In `@server/hooks/persona.ts`:
- Around line 285-297: The handler is calling allower() on every webhook
invocation which recreates expensive GCP KMS clients and resolves the HSM
account; add a module-level lazy singleton (e.g., declare let allowerPromise:
ReturnType<typeof allower> | undefined and implement function getAllower() { if
(!allowerPromise) allowerPromise = allower(); return allowerPromise; }) and
replace calls to await allower() inside the firewallAddress branch with const
allowerClient = await getAllower(); so the KeyManagementServiceClient and
resolved account are created once and reused by exaSend; keep existing error
handling (captureException and c.json) unchanged.
In `@server/test/hooks/activity.test.ts`:
- Line 152: The test currently uses the wrong assertion: replace the assertion
that references sendPushNotification and not.toHaveBeenCalledOnce() with an
assertion that explicitly asserts zero calls (use not.toHaveBeenCalled()) so the
test fails if sendPushNotification is invoked any number of times; update the
expectation in the test containing sendPushNotification to
expect(sendPushNotification).not.toHaveBeenCalled() to ensure it asserts "never
called."
In `@server/test/hooks/persona.test.ts`:
- Around line 694-699: The test uses vi.waitFor with a 500ms timeout to assert
exaSendSpy was not called, which forces waiting the full timeout; update the
assertion to avoid long waits by either lowering the waitFor timeout/interval
(e.g., 100–200ms) in the vi.waitFor call or, better, remove the negative wait
and instead await a deterministic later event or promise that signifies the
operation completed, then assert expect(exaSendSpy).not.toHaveBeenCalled();
locate the vi.waitFor usage and exaSendSpy reference in the test to make the
change.
In `@server/test/utils/allower.test.ts`:
- Around line 55-61: Remove the duplicate file-scoped cspell directive by
keeping a single "// cspell:ignore unstub" comment and deleting the other;
specifically, leave one comment adjacent to the vi.unstubAllEnvs() call (e.g.,
in the first cleanup block) and remove the duplicate in the other block so only
one file-scoped directive remains for vi.unstubAllEnvs().
In `@server/utils/allower.ts`:
- Line 26: Remove the single-use constant gcpKmsKeyName and inline its value at
the call site where it’s used (replace references to gcpKmsKeyName with the
string literal "allower"), then delete the const gcpKmsKeyName = "allower";
declaration; ensure no other references to gcpKmsKeyName remain (search for the
identifier) and run tests/build to verify.
In `@server/utils/persona.ts`:
- Line 74: assetsToPoke is declared as { asset: Address; market: Address | null
}[] but only ETH should have market=null, and the subsequent poke call (where
market is passed) lacks a null check; tighten the typing or add a runtime guard:
change assetsToPoke to a union discriminated by asset (e.g., { asset: ETH;
market: null; ... } | { asset: Address; market: Address; ... }) or keep the
current type and before invoking the poke function check market !== null and
call the ETH-specific poke variant when asset === ETH; locate symbols
assetsToPoke, the code path that builds entries from the Map and the poke
invocation to implement the chosen fix.
- Around line 124-161: The poke retry logic currently calls captureException
inside withRetry's shouldRetry for pokePromises and then again after
Promise.allSettled (results), causing duplicate Sentry reports; update
shouldRetry in the withRetry call used for pokePromises to either remove
captureException or downgrade it to captureException(error, { level: "warning"
}) so only the final rejection handler that iterates over results (the
allSettled loop) reports errors at error level via
captureException(result.reason), ensuring unique/error-level reports come only
from the final handler while retries only emit warnings.
- Around line 71-72: Hoist the constants WETH and ETH to module scope instead of
recreating them per call: move the v.parse(Address, wethAddress) and
v.parse(Address, "0xeeee...") expressions out of the local scope and assign them
to top-level consts named WETH and ETH; update any local references in
persona-related functions to use these module-level WETH and ETH identifiers
(they are currently created via v.parse and use the Address type and wethAddress
constant). Also consider removing duplication by importing the same module-level
ETH/WETH from activity.ts (or extracting them to a shared constants module) so
both persona-related code and activity.ts reference a single source of truth.
In `@server/vitest.config.mts`:
- Around line 57-60: The GCP_* environment variables (GCP_KMS_KEY_RING,
GCP_KMS_KEY_VERSION, GCP_PROJECT_ID, GCP_BASE64_JSON) were appended at the end
of the env list; move them to their alphabetically sorted location between
EXPO_PUBLIC_ALCHEMY_API_KEY and INTERCOM_IDENTITY_KEY so the env block remains
sorted—insert the four GCP_* entries in alphabetical order by key in the
vitest.config.mts env object without changing their values.
In `@src/utils/persona.ts`:
- Line 69: The onError callbacks in startMantecaKYC should use the explicit
unknown type for their error parameter to match the pattern in persona.ts (where
onError: (error: unknown) => {...}) — update the onError signatures inside
startMantecaKYC to use (error: unknown) and adjust any narrowings/casts inside
those handlers before accessing properties so TypeScript stays type-safe; look
for startMantecaKYC and each onError handler to make the change.
5d332da to
85bc66c
Compare
aguxez
force-pushed
the
kyc-poke
branch
4 times, most recently
from
5429589 to
5646eea
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| await keeper | ||
| .poke(account, { ignore: [`NotAllowed(${account})`] }) | ||
| .catch((error: unknown) => captureException(error)); |
There was a problem hiding this comment.
🚩 Behavioral change: poke failures no longer block autoCredit
The old code threw on poke failure (activity.ts old lines 202-205), which prevented autoCredit from running and marked the span as poke_failed. The new code at server/hooks/activity.ts:156-158 catches poke errors via .catch() and continues to autoCredit regardless. This means:
autoCreditnow runs even when all poke operations fail- The span can be marked
SPAN_STATUS_OKeven if poke failed - The outer
Promise.allSettledatserver/hooks/activity.ts:197sees the result as "fulfilled" even when poke failed
This appears intentional (poke is now best-effort), but it changes the observability model — poke failures are no longer surfaced as activity failures in Sentry transaction spans.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const pokePromises = assetsToPoke.map(({ asset, market }) => | ||
| withRetry( | ||
| () => | ||
| base.exaSend( | ||
| { | ||
| name: "poke account", | ||
| op: "exa.poke", | ||
| attributes: { account: accountAddress, asset }, | ||
| }, | ||
| asset === ETH | ||
| ? { | ||
| address: accountAddress, | ||
| abi: combinedAccountAbi, | ||
| functionName: "pokeETH", | ||
| } | ||
| : { | ||
| address: accountAddress, | ||
| abi: combinedAccountAbi, | ||
| functionName: "poke", | ||
| args: [market], | ||
| }, | ||
| ...(options?.ignore ? [{ ignore: options.ignore }] : []), | ||
| ), | ||
| { | ||
| delay: 2000, | ||
| retryCount: 5, | ||
| shouldRetry: ({ error }) => { | ||
| captureException(error, { level: "warning" }); | ||
| return true; | ||
| }, | ||
| }, | ||
| ), | ||
| ); | ||
|
|
||
| const results = await Promise.allSettled(pokePromises); | ||
| for (const result of results) { | ||
| if (result.status === "rejected") captureException(result.reason); |
There was a problem hiding this comment.
🚩 Duplicate error reporting in poke retry chain
When a non-ignored poke error occurs, it gets reported multiple times through the retry chain:
baseExtender.ts:167-175—exaSend's catch block callscaptureExceptionwith "error" level and fingerprinting on EACH attemptkeeper.ts:141-143—withRetry'sshouldRetrycallscaptureExceptionwith "warning" level on each retrykeeper.ts:150-151— afterPromise.allSettled, the rejected result is captured again
With retryCount: 5, a single persistent poke error generates up to 12 Sentry events (6 from exaSend × 6 attempts + 6 from shouldRetry + 1 from allSettled). The old code in activity.ts had a similar issue but only in shouldRetry (exaSend didn't have its own catch-and-report). The baseExtender extraction introduced the additional reporting layer.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async onFetchRequest(request) { | ||
| try { | ||
| captureRequests(parse(Requests, await request.clone().json())); | ||
| } catch (error: unknown) { | ||
| captureMessage("failed to parse or capture rpc requests", { | ||
| level: "error", | ||
| extra: { error }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚩 request.json() vs request.clone().json() inconsistency
The allower at server/utils/allower.ts:101 correctly uses request.clone().json() to avoid consuming the Request body before it's sent by viem's HTTP transport. However, server/utils/keeper.ts:33 uses request.json() without cloning — a pre-existing pattern also found in publicClient.ts:16 and ensClient.ts:14.
If viem's onFetchRequest receives the same Request object used for the actual fetch() call, consuming the body via .json() would prevent the fetch from working. The existing code apparently works (tests pass, production works), suggesting viem internally handles this. The allower's .clone() is the safer approach and arguably the rest of the codebase should adopt it.
Was this helpful? React with 👍 or 👎 to provide feedback.
| keeper | ||
| .poke(account.output, { | ||
| notification: { | ||
| headings: { en: "Account assets updated" }, | ||
| contents: { en: "Your funds are ready to use" }, | ||
| }, | ||
| }) | ||
| .catch((error: unknown) => captureException(error, { level: "error" })); |
There was a problem hiding this comment.
🚩 Persona poke doesn't pass ignore for NotAllowed, relies on prior allow
In server/hooks/persona.ts:331-338, the keeper.poke call after KYC does NOT pass ignore: ['NotAllowed(...)'], unlike the activity handler at server/hooks/activity.ts:157. This is correct because the firewall allow at server/hooks/persona.ts:290-298 should have already authorized the account before poke runs. However, this creates an implicit ordering dependency: if allow succeeds but poke runs before the allow transaction is confirmed on-chain, NotAllowed could occur. The withRetry in poke (5 retries, 2s delay) provides some tolerance for this race, but the error would be reported as a failure rather than silently ignored.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (firewallAddress) { | ||
| try { | ||
| const allowerClient = await getAllower(); | ||
| const account = parse(Address, credential.account); | ||
| await allowerClient.allow(account, { ignore: [`AlreadyAllowed(${account})`] }); | ||
| } catch (error: unknown) { | ||
| captureException(error, { level: "error" }); | ||
| return c.json({ code: "firewall error" }, 500); | ||
| } |
There was a problem hiding this comment.
🚩 Firewall allow returns 500 which triggers Persona webhook retry
At server/hooks/persona.ts:296-297, firewall errors return HTTP 500. Persona webhooks typically retry on non-2xx responses. If the error is transient (e.g., KMS timeout), the retry is beneficial. But if it's a persistent error (e.g., misconfigured firewall address), it will cause repeated retries and repeated allow attempts. The AlreadyAllowed ignore at line 294 handles the case where a retry succeeds on allow but the previous allow had actually gone through. However, the TODO at line 301 suggests the error handling strategy for non-retryable errors hasn't been finalized yet.
Was this helpful? React with 👍 or 👎 to provide feedback.
aguxez
force-pushed
the
kyc-poke
branch
3 times, most recently
from
0f03dcd to
8e6f8ff
Compare
aguxez
force-pushed
the
kyc-poke
branch
2 times, most recently
from
2b10a61 to
936b45d
Compare
2 participants
closes #643
closes #598
Summary by CodeRabbit
New Features
Improvements
Chores / Tests