Conversation
🦋 Changeset detectedLatest commit: 9e783d7 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 |
WalkthroughAdds GCP KMS support and an allower client, centralizes per-account processing in the activity hook to ensure deployment → poke → autoCredit, integrates a new pokeAccountAssets utility, updates persona hook to use firewall/allower and asset poking, and adds tests and config for GCP and poking flows. Changes
Sequence Diagram(s)sequenceDiagram
participant ActivityHook as "Activity Hook"
participant AccountFactory as "Account Factory"
participant Keeper as "Keeper/Wallet"
participant AssetService as "pokeAccountAssets"
participant Notifications as "Push Service"
ActivityHook->>AccountFactory: ensureDeployed(account)
AccountFactory->>Keeper: create/deploy account if needed
Keeper-->>AccountFactory: deployment result
AccountFactory-->>ActivityHook: deployed
ActivityHook->>AssetService: pokeAccountAssets(account, options)
AssetService->>Keeper: read balances / market mappings
Keeper-->>AssetService: balance data
AssetService->>Keeper: exaSend poke ETH/ERC-20 (batched)
Keeper-->>AssetService: poke results
ActivityHook->>Keeper: autoCredit(account)
Keeper-->>ActivityHook: credit result
ActivityHook->>Notifications: send notification (if applicable)
Notifications-->>ActivityHook: notified
sequenceDiagram
participant PersonaHook as "Persona Hook"
participant Allower as "Allower / Firewall"
participant AssetService as "pokeAccountAssets"
participant Keeper as "Keeper/Wallet"
PersonaHook->>Allower: get client / authorize(account, token)
Allower-->>PersonaHook: success / error
alt account parsed successfully
PersonaHook->>AssetService: pokeAccountAssets(account)
AssetService->>Keeper: read balances & poke
Keeper-->>AssetService: results
AssetService-->>PersonaHook: 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.
|
✅ All tests passed. |
![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
aguxez
force-pushed
the
kyc-poke
branch
2 times, most recently
from
b77bb00 to
8b80e9b
Compare
cruzdanilo
force-pushed
the
main
branch
6 times, most recently
from
d4d2407 to
5d070cb
Compare
cruzdanilo
force-pushed
the
main
branch
3 times, most recently
from
f322d71 to
7337c14
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/package.json (1)
35-47:⚠️ Potential issue | 🟠 Major@google-cloud/kms version needs correction.
The specified version^5.3.0cannot resolve to any stable release (latest stable is 5.1.0). Update to^5.1.0or check if a newer release is available.
The@valora/viem-account-hsm-gcp@^1.2.16version is current.
🤖 Fix all issues with AI agents
In `@server/utils/allower.ts`:
- Around line 85-103: The default export function createAllower in allower.ts
should be renamed to match the filename; rename the function signature and any
internal references from createAllower to allower (or alternatively change the
file name to match createAllower), then update all import sites that reference
createAllower to use allower so the default export name and filename stay
consistent; ensure the returned createWalletClient(...).extend(extender) logic
and any usages still work after the rename.
In `@server/utils/gcp.ts`:
- Around line 4-15: Remove the explanatory header and inline comments in this
file and replace them only with allowed static-analysis annotations or
TODO/FIXME markers; specifically delete prose around the DECODING_ITERATIONS
constant, GOOGLE_APPLICATION_CREDENTIALS declaration, the block explaining why
the file exists, and the initializationPromise comment while keeping any
necessary TODO/FIXME or allowed annotations, and ensure the
resetGcpInitialization function comment area is similarly cleaned up so only
permitted annotations remain.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/hooks/activity.ts (1)
108-123:⚠️ Potential issue | 🟠 MajorDon’t skip account processing for market-share transfers.
The earlycontinuepreventsaccountsToProcessfrom being populated, so poke/autoCredit never runs for those transfers.🐛 Suggested fix
for (const { toAddress: account, rawContract, value, asset: assetSymbol } of transfers) { if (!accounts[account]) continue; - // skip notifications for market share transfers - if (rawContract?.address && markets.has(rawContract.address)) continue; + accountsToProcess.add(account); + // skip notifications for market share transfers + if (rawContract?.address && markets.has(rawContract.address)) continue; const asset = rawContract?.address ?? ETH; const underlying = asset === ETH ? WETH : asset; sendPushNotification({ userId: account, headings: { en: "Funds received" }, contents: { en: `${value ? `${value} ` : ""}${assetSymbol} received${marketsByAsset.has(underlying) ? " and instantly started earning yield" : ""}`, }, }).catch((error: unknown) => captureException(error)); - accountsToProcess.add(account); }
🤖 Fix all issues with AI agents
In @.changeset/kind-ants-beam.md:
- Around line 1-5: The changeset currently includes a package patch and a
human-facing message for a test-only fix; replace the body of
.changeset/kind-ants-beam.md with an empty changeset (only the three `---`
separators) so it does not trigger a version bump for "@exactly/server" — remove
the package line and the "🐛 fix chain mock in tests" description and leave only
the minimal empty changeset content.
In `@server/test/utils/allower.test.ts`:
- Around line 20-24: The mock in mockViemHsm returns an invalid Ethereum address
("0xGCPAccount"); update the gcpHsmToAccount mock return value to use a valid
42-character hex address (0x + 40 hex chars) so future tests that validate
address format won't fail—locate the mockViemHsm function and change the address
string in the gcpHsmToAccount mockResolvedValue to a properly formatted hex
address.
.changeset/kind-ants-beam.md
Outdated
| --- | ||
| "@exactly/server": patch | ||
| --- | ||
|
|
||
| 🐛 fix chain mock in tests |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using an empty changeset for test-only changes.
This changeset describes a test fix which is not user-facing. Based on learnings, non-user-facing changes should use empty changesets (containing only --- separators) since they don't warrant a version bump.
♻️ Proposed change for non-user-facing test fix
---
-"@exactly/server": patch
---
-
-🐛 fix chain mock in tests📝 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.
| --- | |
| "@exactly/server": patch | |
| --- | |
| 🐛 fix chain mock in tests | |
| --- | |
| --- |
🤖 Prompt for AI Agents
In @.changeset/kind-ants-beam.md around lines 1 - 5, The changeset currently
includes a package patch and a human-facing message for a test-only fix; replace
the body of .changeset/kind-ants-beam.md with an empty changeset (only the three
`---` separators) so it does not trigger a version bump for "@exactly/server" —
remove the package line and the "🐛 fix chain mock in tests" description and
leave only the minimal empty changeset content.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@server/hooks/activity.ts`:
- Around line 108-112: Remove the inline explanatory comment "// skip
notifications for market share transfers" from the transfers loop; locate the
for...of loop iterating over transfers where variables are destructured
(account/toAddress, rawContract, value, assetSymbol) and referencing
accounts[account] and markets.has(rawContract.address), and simply delete that
comment so the TS file contains no explanatory inline comments (do not replace
with non-allowed comment types; if a static annotation is required use one of
the allowed markers).
In `@server/test/hooks/persona.test.ts`:
- Around line 403-405: Remove the explanatory inline comment in the afterEach
block; leave the cleanup statement as-is (the afterEach handler containing
database.update(credentials).set({ pandaId: null }).where(eq(credentials.id,
"persona-ref"))) but delete the comment "// reset pandaId to null for next test"
so the TS file contains no explanatory comments per guidelines.
- Around line 466-703: Remove all inline explanatory comments in the tests
(e.g., comments after mockResolvedValueOnce/readContractSpy, getBalance mocks,
and expectation blocks such as “assets from …”, “balanceOf …”, and “should poke
…”) across the tests in persona.test.ts (including the test cases that mock
readContract/getBalance and assert exaSend calls). Leave only allowed
annotations (`@ts-expect-error`, eslint/slither/solhint/cspell disables) and
TODO/FIXME markers; do not change test logic, variable names (e.g.,
readContractSpy, exaSendSpy, publicClient.getBalance, deriveAddress), or
assertions—just delete the explanatory comment text.
In `@server/vitest.config.mts`:
- Around line 57-60: Replace the hard-coded GCP_BASE64_JSON value with a clearly
dummy placeholder and allow runtime override from env.GCP_BASE64_JSON: update
the configuration where GCP_BASE64_JSON is defined (in vitest config) to use a
non-sensitive dummy string like "DUMMY_BASE64_SERVICE_ACCOUNT" and read the real
value from process.env.GCP_BASE64_JSON when present so CI/local runs can inject
secrets at runtime; ensure the symbol GCP_BASE64_JSON in the config is only a
fallback placeholder and not a real service-account payload.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/test/hooks/activity.test.ts`:
- Around line 156-175: Replace the mock that triggers the ESLint
unicorn/no-useless-undefined rule by removing the explicit undefined: in the
test where you create pokeAccountAssetsSpy via vi.spyOn(persona,
"pokeAccountAssets").mockResolvedValue(undefined), change it to use
.mockResolvedValue() so pokeAccountAssetsSpy is still resolved but no useless
undefined literal is passed; keep the rest of the test (response assertion and
expect(pokeAccountAssetsSpy).toHaveBeenCalledWith(...)) unchanged.
1cecb92 to
3ace7ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/activity.test.ts (1)
132-154:⚠️ Potential issue | 🟡 MinorIncorrect assertion:
not.toHaveBeenCalledOnce()doesn't mean "not called".Line 152 uses
expect(sendPushNotification).not.toHaveBeenCalledOnce()which passes if the spy was called zero times OR more than once. For asserting "not called", use.not.toHaveBeenCalled().🐛 Suggested fix
- expect(sendPushNotification).not.toHaveBeenCalledOnce(); + expect(sendPushNotification).not.toHaveBeenCalled();
🤖 Fix all issues with AI agents
In `@server/utils/gcp.ts`:
- Around line 116-137: The current trackKmsOperation sets tags inside withScope
for both success and failure, but tags only apply to captured events in that
scope; remove the success branch's scope usage and only create a scoped event
for failures: keep withScope(...) and the captureException/captureMessage path
for when success === false (using scope.setTag for operation/result/success
there), and for success simply avoid withScope (or optionally emit a non-scoped
breadcrumb or a plain captureMessage if you want a recorded success); update the
function trackKmsOperation and references to withScope, captureException, and
captureMessage accordingly so tags are only set within the scope that actually
captures an event.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/package.json`:
- Line 35: The package.json lists a non-existent version for the dependency
"@google-cloud/kms" (^5.3.0); update the dependency entry for
"@google-cloud/kms" to an existing released version (for example "^5.2.1" or the
current latest on npm) so installs succeed, keeping the dependency name and
alphabetical ordering intact; modify the "dependencies" entry for
"@google-cloud/kms" accordingly and run an install to verify.
| "dependencies": { | ||
| "@account-kit/infra": "catalog:", | ||
| "@exactly/lib": "^0.1.0", | ||
| "@google-cloud/kms": "^5.3.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat server/package.jsonRepository: exactly/exa
Length of output: 6157
🌐 Web query:
@google-cloud/kms latest version npm registry
💡 Result:
The latest published version of @google-cloud/kms on the npm registry is 5.2.1. [1][2]
🌐 Web query:
@valora/viem-account-hsm-gcp latest version npm registry
💡 Result:
The latest version of @valora/viem-account-hsm-gcp on the npm registry is 1.2.16. [1]
Update @google-cloud/kms to an existing version. The dependency specifies ^5.3.0, but the latest published version on npm is 5.2.1. This will cause installation failures since version 5.3.0 does not exist. Update to ^5.2.1 or the latest available version.
Alphabetical ordering is correct for both dependencies: @google-cloud/kms and @valora/viem-account-hsm-gcp are placed in proper alphabetical positions per coding guidelines.
🤖 Prompt for AI Agents
In `@server/package.json` at line 35, The package.json lists a non-existent
version for the dependency "@google-cloud/kms" (^5.3.0); update the dependency
entry for "@google-cloud/kms" to an existing released version (for example
"^5.2.1" or the current latest on npm) so installs succeed, keeping the
dependency name and alphabetical ordering intact; modify the "dependencies"
entry for "@google-cloud/kms" accordingly and run an install to verify.
2 participants
closes #643
closes #598
Summary by CodeRabbit
New Features
Improvements
Chores