✨ server: add allower account with gcp kms #666
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 8b80e9b 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 |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds on‑chain asset discovery and batched poking via a new pokeAccountAssets utility; integrates it into persona and activity hooks with centralized per‑account processing (including account creation), and introduces GCP KMS "allower" support plus related GCP utilities, tests, env and dependency wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as Webhook (persona/activity)
participant Factory as Account Factory
participant Util as pokeAccountAssets
participant RPC as On‑Chain RPC
participant Keeper as Keeper (exaSend)
participant Service as AutoCredit/Notifications
Hook->>Factory: ensure account exists (create with publicKey if undeployed)
alt account exists or created
Hook->>Util: pokeAccountAssets(accountAddress, {ignore, notification})
Util->>RPC: fetch enabled assets & markets
RPC-->>Util: assets list
loop per asset
Util->>RPC: read balance (ETH or ERC‑20)
RPC-->>Util: balance
alt balance > 0
Util->>Keeper: exaSend(poke request)
Keeper-->>Util: receipt
end
end
Util-->>Hook: poke results
Hook->>Service: run autoCredit, activate card/credentials, send notifications
Service-->>Hook: status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 65.82% 77.95% +12.12%
===========================================
Files 190 55 -135
Lines 5996 2631 -3365
Branches 1734 590 -1144
===========================================
- Hits 3947 2051 -1896
+ Misses 1870 377 -1493
- Partials 179 203 +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.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/hooks/persona.ts`:
- Line 5: The file mixes a namespace import (import * as v from "valibot") with
named imports; pick one style—prefer named imports—by removing the namespace
import and consolidating to a single import listing the used valibot symbols
(e.g., object, optional, number, boolean, defaulted, parse, etc.), then update
any references like v.object or v.optional to the direct identifiers (object,
optional, ...) used elsewhere in persona.ts so all valibot usage is consistent.
b3ddb6f to
e9ed433
Compare
aguxez
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/persona.ts (1)
61-66: 🧹 Nitpick | 🔵 TrivialUse object method shorthand for the multi-line
onErrorhandler.The multi-line arrow function violates the style rule preferring function declarations for multi-line functions. Rather than extracting to a separate function, use method shorthand syntax directly in the object configuration for better idiomaticity and closure access:
♻️ Suggested refactor
onError: (error: unknown) => { signal.removeEventListener("abort", onAbort); client.destroy(); reportError(error); reject(new Error("persona inquiry failed", { cause: error })); },Change to:
onError(error: unknown) { signal.removeEventListener("abort", onAbort); client.destroy(); reportError(error); reject(new Error("persona inquiry failed", { cause: error })); },
🤖 Fix all issues with AI agents
In @.do/app.yaml:
- Around line 84-96: The deployment manifest is missing assurance that required
GCP environment variables/secrets (GCP_KMS_KEY_RING, GCP_KMS_KEY_VERSION,
GCP_PROJECT_ID, GCP_BASE64_JSON) are provisioned for staging/preview; update
your ops/runbook and CI/CD setup to create or sync these runtime env vars and
the SECRET GCP_BASE64_JSON into staging and preview environments and add a short
checklist entry to documentation confirming their names and required scope/type
so ops can verify and reprovision them as needed.
In `@server/hooks/persona.ts`:
- Around line 285-297: The firewall block currently catches errors and returns
c.json(...) which bypasses Hono's middleware; instead, after capturing the error
with captureException(error, { level: "error" }), throw a new HTTPException(500,
"firewall error") so Hono can handle it—update the catch in the firewallAddress
branch (surrounding allower(), allowerClient.exaSend and the existing
captureException call) to throw HTTPException rather than returning c.json,
preserving the original error logged by captureException.
In `@server/test/utils/allower.test.ts`:
- Around line 62-73: The test re-invokes privateKeyToAccount(privateKey) just to
get the expected address; instead capture the address returned by the mock when
getAccount calls it and assert against that to avoid duplicate computation.
After calling getAccount(), read the mock's return value (e.g. from
(privateKeyToAccount as Mock).mock.results[0].value.address or the mock's last
result) into expectedAddress and use
expect(account.address).toBe(expectedAddress); keep the existing checks for
gcpHsmToAccount and nonceManager unchanged.
In `@server/utils/allower.ts`:
- Around line 32-82: getAccount currently reads process.env directly; refactor
it to accept a configuration object (e.g., Config or AllowerConfig) instead of
accessing process.env, and update createAllower (or whichever caller constructs
allower) to load environment variables once at startup and pass that config in.
Specifically: remove direct uses of GCP_PROJECT_ID, GCP_KMS_KEY_VERSION,
GCP_KMS_KEY_RING, GCP_BASE64_JSON/GOOGLE_APPLICATION_CREDENTIALS,
KEEPER_PRIVATE_KEY inside getAccount, add a typed parameter (e.g., config: {
gcpProjectId?, gcpKmsKeyVersion?, gcpKmsKeyRing?, gcpKmsKeyName?,
googleCredentialsPath?, keeperPrivateKey? }), and use those fields in getAccount
(and when constructing KeyManagementServiceClient and calling
gcpHsmToAccount/privateKeyToAccount); update callers (createAllower) to read
process.env once at startup and pass the populated config and nonceManager into
getAccount.
In `@server/utils/gcp.ts`:
- Around line 19-47: The function initializeGcpCredentials currently reads
process.env directly; change it to accept an injected config object (e.g., add a
parameter like config or gcpConfig) and use that instead of process.env inside
initializeGcpCredentials and its helper calls (retain hasCredentials,
DECODING_ITERATIONS, GOOGLE_APPLICATION_CREDENTIALS symbols). Update the logic
to read config.GCP_BASE64_JSON and config.GCP_PROJECT_ID, keep the same
decoding/write behavior and error handling, and ensure callers pass the loaded
configuration at app startup (adjust any call sites to provide the config) so no
direct process.env access remains in this utility.
44e06fc to
fe81586
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@server/package.json`:
- Line 35: The package.json currently pins "@google-cloud/kms" to "^5.2.1";
update that dependency to "^5.3.0" to use the latest stable release. Open
package.json, locate the dependency entry for "@google-cloud/kms" and change its
version string to "^5.3.0", then run your package manager (npm/yarn) to refresh
the lockfile and verify tests/build succeed.
In `@server/test/utils/gcp.test.ts`:
- Around line 23-31: Add a new unit test that verifies
initializeGcpCredentials() returns early when credentials already exist by
stubbing GCP_BASE64_JSON and making hasCredentials()/the underlying mockAccess
resolve (e.g., mockAccess.mockResolvedValue(undefined)), then calling
initializeGcpCredentials() and asserting mockWriteFile was not called; reference
initializeGcpCredentials, hasCredentials (or the test's mockAccess and
mockWriteFile) so the test mirrors the provided example and covers the skip
branch.
In `@server/utils/allower.ts`:
- Around line 55-67: Replace the hardcoded region string "us-west2" in the
gcpHsmToAccount call with a parameter read from the new environment variable
GCP_KMS_LOCATION (use the existing projectId, gcpKmsKeyRing, gcpKmsKeyName,
version variables to build the KMS path) so gcpHsmToAccount receives a fully
parameterized hsmKeyVersion; also review the withRetry invocation (used with
withRetry and isRetryableKmsError) and either change retryCount: 3 to match the
retryCount: 5 used in persona.ts or make the retry count configurable via an env
var (e.g., GCP_KMS_RETRY_COUNT) and wire that into the withRetry options,
documenting the chosen value where appropriate.
- Around line 84-96: The onFetchRequest hook in createAllower currently consumes
the fetch body by calling await request.json(), causing the actual RPC call to
fail; change the call inside the async onFetchRequest handler to await
request.clone().json() and pass that parsed body to
captureRequests(parse(Requests, ...)). Apply the same fix to the other similar
hooks found in keeper.ts, publicClient.ts, ensClient.ts, and traceClient.ts so
every onFetchRequest uses request.clone().json() instead of request.json(); keep
the surrounding symbols intact (createAllower, onFetchRequest, captureRequests,
Requests) and ensure the clone is awaited before parsing.
In `@server/utils/gcp.ts`:
- Around line 134-138: The captureException call includes an unnecessary empty
tags object; remove the tags: {} property from the captureException(...)
invocation (the call near the captureException(error, { level: "error", tags: {}
}) line) so it simply passes the level (or no second arg) and relies on
scope.setTag() for tag values—i.e., delete the tags field from the options to
avoid the redundant empty object.
- Around line 75-77: Remove the redundant truthy check for
process.env.GCP_PROJECT_ID in the validation branch inside server/utils/gcp.ts:
since the variable is already asserted earlier, change the if that currently
reads `if (process.env.GCP_PROJECT_ID &&
!/^[a-z][a-z0-9-]{4,28}[a-z0-9]$/.test(process.env.GCP_PROJECT_ID))` to only
test the regex (e.g., `if
(!/^[a-z][a-z0-9-]{4,28}[a-z0-9]$/.test(process.env.GCP_PROJECT_ID))`) so the
validation for GCP_PROJECT_ID format still pushes the same error message to
errors without the redundant existence check.
In `@server/utils/persona.ts`:
- Around line 61-71: The repeated retry options object used in the withRetry
calls (delay: 2000, retryCount: 5, shouldRetry: ({ error }) => {
captureException(error, { level: "error" }); return true; }) should be extracted
into a shared constant (e.g. DEFAULT_RETRY_OPTIONS) and reused where withRetry
is invoked (such as the call that assigns marketsByAsset and the other similar
readContract calls around the file); keep the shouldRetry implementation intact
so captureException still runs, import or define the constant near the top of
persona.ts, and replace the inline retry objects with the shared constant when
calling withRetry for publicClient.readContract and similar usages.
- Line 59: The constant combinedAccountAbi (built from exaPluginAbi,
upgradeableModularAccountAbi, auditorAbi, marketAbi) is being recreated inside a
function; hoist it to module scope as a top-level const so the array spread
happens once, not on every call—create a module-level const combinedAccountAbi =
[...exaPluginAbi, ...upgradeableModularAccountAbi, ...auditorAbi, ...marketAbi]
and update any functions that reference the inner variable to use this top-level
symbol.
- Around line 88-99: The ERC-20 readContract call for balanceOf
(publicClient.readContract with erc20Abi using args [accountAddress]) currently
catches a single error and returns 0n, which can hide transient RPC failures;
add retry logic similar to the ETH balance fetch: wrap the readContract call in
a retry loop (e.g., maxAttempts = 3 with short backoff), attempt to read
balanceOf up to the max attempts, captureException on each failure (include
attempt info), and only after exhausting retries return { asset, market,
balance: 0n }; keep identifiers balance, asset, market, accountAddress,
erc20Abi, publicClient.readContract and captureException to locate and update
the code.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@cspell.json`:
- Line 205: The single-line array for ignoreRegExpList hurts diff reviewability;
update the JSON so the "ignoreRegExpList" value is formatted as a multi-line
array with each regex on its own line (preserve the same regex strings and
ordering) so future additions/removals only change a single element; locate the
"ignoreRegExpList" entry in cspell.json and reformat its array accordingly.
In `@server/hooks/activity.ts`:
- Around line 126-190: The span created in startSpan (inside
continueTrace/withScope) closes before the autoCredit promise resolves because
autoCredit is invoked without awaiting it; move the autoCredit call to be
awaited inside the same async span callback (e.g., await autoCredit(account)),
wrap it in try/catch to capture errors (use captureException and set
span.setStatus({ code: SPAN_STATUS_ERROR, message: "autoCredit_failed" }) on
failure), and keep subsequent work that sets span attributes
(span.setAttribute("exa.autoCredit", ...), span.setAttribute("exa.card", ...),
span.setAttribute("exa.mode", 1), and sendPushNotification) inside that awaited
flow so attributes and status are set before the span finishes; ensure you still
catch/send errors from sendPushNotification and pokeAccountAssets as before.
In `@server/utils/allower.ts`:
- Around line 84-95: In createAllower's transport.onFetchRequest handler, guard
the parse call so a thrown error cannot break the RPC: wrap the await
request.clone().json() and parse(Requests, ...) and captureRequests(...) call in
a try/catch inside onFetchRequest (in function createAllower) and on error log
the parsing/telemetry failure (use the module logger if available or
console.error) but do not rethrow—ensure the handler returns/finishes normally
so the RPC request proceeds.
In `@server/utils/gcp.ts`:
- Around line 122-140: trackKmsOperation currently ignores non-Error failures;
update it so all failures are reported by normalizing the error before sending
to Sentry: when success is false, if error is an Error use
captureException(error) as now, otherwise call captureException(new
Error(String(error))) or captureMessage(String(error), { level: "error", extra:
{ originalError: error } }) and preserve the existing tags (kms.operation.type,
kms.operation.success, kms.operation.result) inside the withScope block so
non-Error values are captured with the same context.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/hooks/activity.ts`:
- Around line 126-131: Rename the variable accountData to a more specific name
(e.g., accountInfo or accountEntry) wherever it's used in the map over
[...accountsToProcess]; update the destructuring line to `const { publicKey,
factory } = accountInfo;` and the null check to `if (!accountInfo) return null;`
as well as the return to `return [account, { publicKey, factory }] as const;` so
all references (accountsToProcess map callback, accounts lookup, and
destructuring) reflect the new name.
In `@server/utils/gcp.ts`:
- Around line 122-128: The Sentry tag calls in trackKmsOperation are being made
at global scope via setTag("kms.operation.type", ...) and
setTag("kms.operation.success", ...) which can leak to unrelated spans; move all
setTag calls into a Sentry.withScope callback so the tags are scoped to the
current operation (both success and failure branches), e.g., call withScope and
inside it set the "kms.operation.type", "kms.operation.success", and
"kms.operation.result"/error-related tags, then capture the event or log so tags
do not persist globally.
- Around line 4-5: Change all inline and block comments in this module to
lowercase to comply with coding guidelines; specifically update the comment
above the DECODING_ITERATIONS constant ("// tokens/credentials are
base64-encoded multiple times by deployment tooling") and any other comments
near the constants or functions referenced in this diff (the comments around
DECODING_ITERATIONS and the subsequent comment blocks noted in the review) so
every letter is lowercase while preserving the same wording and punctuation.
cruzdanilo
force-pushed
the
main
branch
5 times, most recently
from
c71e562 to
d4d2407
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #643
closes #598
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores