Conversation
cruzdanilo
requested review from
dieguezguille,
franm91 and
nfmelendez
as code owners
🦋 Changeset detectedLatest commit: 09ea4d9 The changes in this PR will be included in the next version bump. 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 an i18n aria-label to the loan amount input; introduces a Maestro borrowAsset subflow and updates local flow; extends Anvil typings and on-chain log processing; implements e2e multi-tx send/status handling; expands e2e/docker test infra and persona mocks; and updates test scripts/package.json. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as TestRunner
participant Maestro as Maestro
participant UI as UI
participant Server as Server
participant Anvil as Anvil
TestRunner->>Maestro: start local flow
Maestro->>Maestro: evalScript -> capture portfolioBefore
Maestro->>UI: run subflow borrowAsset (taps/inputs/submit)
UI->>Anvil: submit transaction(s)
Anvil-->>UI: tx hashes / mined block
Maestro->>Anvil: eth_blockNumber / eth_getLogs (from..to)
Anvil-->>Maestro: Transfer logs (RpcLog[])
Maestro->>Server: activity(logs) / sync nonces
Maestro->>Maestro: read portfolioAfter
Maestro->>TestRunner: assert portfolioAfter - portfolioBefore == 10
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 @cruzdanilo, 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 introduces a comprehensive end-to-end test for the asset borrowing functionality, ensuring its stability and correctness. It significantly upgrades the local testing infrastructure by incorporating Docker Compose for essential blockchain development tools like Otterscan and Sourcify, streamlining contract verification and debugging. Additionally, it includes an important accessibility improvement for the loan amount selector and refines the e2e wallet interaction handling. 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
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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 adds an end-to-end test for the 'borrow asset' flow. The changes include new Maestro test flows, scripts to interact with the blockchain during tests, and significant improvements to the local e2e testing environment, such as integrating a local block explorer and contract verifier using Docker. Mocks for services like Persona have also been enhanced to support more complex test scenarios. Overall, this is a great step towards improving test coverage and developer experience. I've found one bug in the test utility for contract verification and suggested a fix.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #710 +/- ##
==========================================
+ Coverage 64.59% 65.86% +1.26%
==========================================
Files 173 190 +17
Lines 5387 5996 +609
Branches 1505 1734 +229
==========================================
+ Hits 3480 3949 +469
- Misses 1727 1867 +140
Partials 180 180
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:
|
cruzdanilo
force-pushed
the
e2e
branch
2 times, most recently
from
56b09aa to
81a43a1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/test/anvil.ts (2)
225-307:⚠️ Potential issue | 🟡 MinorAdd retry/readiness checks around Sourcify verification to prevent startup races.
Sourcify service lacks a healthcheck in docker-compose.yaml, so
docker compose up -d --waitreturns before it's ready to handle verification requests. Add retry logic to the verify calls (lines 237–269) to handle temporary unavailability during startup.
1-24:⚠️ Potential issue | 🟡 MinorGuard on docker compose availability, not just daemon status and add missing reason to eslint annotation.
docker infocan succeed even when the compose plugin is missing, so the subsequent compose calls may fail. Additionally, the eslint-disable-line directive at line 202 is missing the required reason explanation per the coding guidelines.Suggested fixes
Line 21:
- const docker = env.NODE_ENV === "e2e" && !env.CI && (await $`docker info`.then(() => true).catch(() => false)); + const docker = env.NODE_ENV === "e2e" && !env.CI && (await $`docker compose version`.then(() => true).catch(() => false));Line 202:
- const files = await readdir(__dirname, { recursive: true }); // eslint-disable-line unicorn/prefer-module + const files = await readdir(__dirname, { recursive: true }); // eslint-disable-line unicorn/prefer-module -- controlled access to test directory
🤖 Fix all issues with AI agents
In @.maestro/flows/local.yaml:
- Around line 50-66: The YAML linter errors are caused by inconsistent spacing
around inline flow mappings and inline comments in the borrow flow block; update
the inline mappings used by evalScript, runFlow (env), runFlow (when), and the
repeat while visible mappings so there is exactly one space after '{' and one
before '}' (e.g., env: { amount: "10", installments: "4" } -> ensure spaces
inside the braces) and ensure inline comments have a single space before the '#'
(e.g., change "- runFlow:# HACK" style to "- runFlow: # HACK"); apply this
spacing consistently for the symbols evalScript, runFlow, env, when, repeat,
while, and visible to satisfy YAMLlint.
In @.maestro/src/script/hookProposals.ts:
- Around line 42-49: The loop currently converts large ERC-20 amounts to Number
which loses precision; change the activity() signature to accept value: bigint |
string and update its implementation and all callers accordingly, then in
.maestro/src/script/hookProposals.ts pass the raw BigInt(log.data) (i.e.,
BigInt(log.data)) from the logs loop instead of Number(BigInt(log.data)); ensure
any downstream code that consumes activity(account, value) handles bigint or
string (e.g., serialization to string for logging/storage) to avoid runtime/type
errors.
- Around line 37-40: Replace the indefinite busy-wait while loop that calls
readContract({ address: proposalManagerAddress, functionName: "nonces", args:
[account], abi: proposalManagerAbi }) < nextNonce with a bounded polling loop:
add a maxRetries counter and iterate up to that limit, calling readContract each
iteration and breaking when the nonce >= nextNonce; if maxRetries is exceeded,
throw or return an error so the caller/orchestration layer can handle retry (or
delegate to runFlow's extendedWaitUntil). Wrap the polling in try/catch around
readContract to surface RPC errors, and reference the symbols readContract,
proposalManagerAddress, proposalManagerAbi, account, and nextNonce when
implementing the change.
In @.maestro/subflows/borrowAsset.yaml:
- Around line 7-32: YAML linter errors are caused by extra spaces inside
inline-mapping braces; tighten spacing for all inline mappings (e.g., change "{
visible: Explore decentralized services }" to "{visible: Explore decentralized
services}", "{ tapOn: Connect wallet to Exactly Protocol }" to "{tapOn: Connect
wallet to Exactly Protocol}", "{ id: Amount }" to "{id: Amount}", and "{
account: "${output.account}" }" to "{account: "${output.account}"}") across the
keys shown (when, commands, tapOn, runFlow, runScript env, etc.); update every
occurrence of inline braces in this subflow so there are no spaces immediately
after "{" or before "}" to satisfy YAMLlint.
In `@server/script/e2e.ts`:
- Line 14: The current spawn call starts Vitest in persistent watch mode (const
v = spawn(...)), which is inappropriate for CI; modify the spawn invocation that
creates v to choose args based on process.env.CI so CI runs a single pass.
Specifically, replace the hardcoded ["--watch"] with a conditional like: const
args = process.env.CI ? ["run"] : ["--watch"]; then call spawn("vitest", args, {
stdio: ["ignore","pipe","pipe"], env: { ...process.env, FORCE_COLOR: "1" } });
keep the variable name v and existing stdio/env options unchanged.
In `@server/test/anvil.ts`:
- Around line 202-222: Replace the inline disable comment after the readdir call
with a preceding eslint-disable-next-line that includes a reason; specifically,
remove the trailing "// eslint-disable-line unicorn/prefer-module" after the
invocation of readdir(...) and add a line above it: "// eslint-disable-next-line
unicorn/prefer-module -- <reason>" (keeping the call to readdir(__dirname, {
recursive: true }) as-is) so the rule is disabled with the required annotation
format.
In `@server/test/docker-compose.yaml`:
- Around line 4-23: Replace the two "latest" images with pinned tags or digests
to ensure reproducible e2e runs: change "image: otterscan/otterscan:latest" and
"image: ghcr.io/ethereum/sourcify/server:latest" to explicit version tags or
immutable digests (e.g., otterscan/otterscan:<version> and
ghcr.io/ethereum/sourcify/server@sha256:<digest>), update any CI/test configs
that assume "latest", and document the chosen tags so tests can be reproduced;
verify by running the e2e suite to confirm no behavior change.
- Around line 41-90: The docker-compose YAML uses compact brace-style mappings
and hardcoded secrets; change all inline brace mappings in services (e.g.,
depends_on entries for postgres, migrations, schema-fix) to explicit block-style
mappings with proper spacing and ensure comments have a preceding space, and
replace hardcoded credentials (POSTGRES_PASSWORD, PGPASSWORD and any
SESSION_SECRET usage) with environment variable references (e.g., use
${POSTGRES_PASSWORD} and ${PGPASSWORD} or ${SESSION_SECRET} with sensible
defaults only for local dev) so secrets are injected from the environment or a
.env file; update the postgres, schema-fix and migrations service environment
fields to reference these variables and ensure healthcheck/depends_on condition
keys remain valid after rebasing to block style.
- Around line 3-20: The otterscan service's OTTERSCAN_CONFIG uses "erigonURL":
"http://localhost:8545" which is unreachable from inside the container; update
the OTTERSCAN_CONFIG in the otterscan service to point erigonURL to
"http://host.docker.internal:8545" (same pattern as the Sourcify service) so the
container can reach the host's Anvil instance.
| process.stderr.on("error", () => null); | ||
| const v = spawn("vitest", ["run"], { stdio: ["ignore", "pipe", "pipe"], env: { ...process.env, FORCE_COLOR: "1" } }); | ||
| const v = spawn("vitest", ["--watch"], { stdio: ["ignore", "pipe", "pipe"], env: { ...process.env, FORCE_COLOR: "1" } }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n server/script/e2e.tsRepository: exactly/exa
Length of output: 1676
🏁 Script executed:
# Check if there are other scripts using similar patterns for CI detection
rg "process\.env\.CI|CI.*spawn|vitest" --type ts --type js -B2 -A2Repository: exactly/exa
Length of output: 12848
🏁 Script executed:
# Check how the e2e script is typically invoked
find . -type f -name "package.json" -o -name "*.yml" -o -name "*.yaml" | head -20Repository: exactly/exa
Length of output: 730
🏁 Script executed:
# Check package.json scripts
rg "e2e" package.json -A2 -B2Repository: exactly/exa
Length of output: 2598
🏁 Script executed:
# Check workflow files for e2e.ts invocation
rg "e2e" .github/workflows/test.yaml -A3 -B3Repository: exactly/exa
Length of output: 1475
🏁 Script executed:
# Search for how vitest watch mode is typically handled
rg "spawn.*vitest|--watch" --type ts --type jsRepository: exactly/exa
Length of output: 197
Gate watch mode to interactive environments; use run mode in CI.
Vitest's --watch mode continuously monitors files and never exits on its own. While the current HTTP shutdown mechanism works, this approach is semantically incorrect—watch mode should only be used for local development. CI environments should use vitest run for single execution. Use process.env.CI (automatically set by GitHub Actions and standard CI systems) to conditionally select the appropriate mode:
Suggested fix
-const v = spawn("vitest", ["--watch"], { stdio: ["ignore", "pipe", "pipe"], env: { ...process.env, FORCE_COLOR: "1" } });
+const v = spawn("vitest", [process.env.CI ? "run" : "--watch"], { stdio: ["ignore", "pipe", "pipe"], env: { ...process.env, FORCE_COLOR: "1" } });🤖 Prompt for AI Agents
In `@server/script/e2e.ts` at line 14, The current spawn call starts Vitest in
persistent watch mode (const v = spawn(...)), which is inappropriate for CI;
modify the spawn invocation that creates v to choose args based on
process.env.CI so CI runs a single pass. Specifically, replace the hardcoded
["--watch"] with a conditional like: const args = process.env.CI ? ["run"] :
["--watch"]; then call spawn("vitest", args, { stdio: ["ignore","pipe","pipe"],
env: { ...process.env, FORCE_COLOR: "1" } }); keep the variable name v and
existing stdio/env options unchanged.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.maestro/flows/local.yaml:
- Around line 50-66: Yamllint is flagging brace and inline-comment spacing in
the borrow block (look for the evalScript line and the three runFlow blocks,
plus the env and when mappings and the repeat/visible entries); fix by adding a
space after every opening brace and before every closing brace in inline
mappings (e.g., "{ amount: \"10\", installments: \"4\" }", "{ true:
\"${maestro.platform != 'web'}\" }", "{ platform: web }", and "{ visible: { id:
Pending proposals } }") and ensure inline comments use a single space before the
"#" and are separated from the preceding value (e.g., "runFlow: # HACK ...");
apply the same spacing rules to the repeat/visible/commands lines so yamllint no
longer reports brace/comment spacing errors.
- Around line 67-68: Add a retry/read loop that invokes the existing
readPortfolio subflow repeatedly before performing the assertTrue check so the
asynchronous portfolio update is observed reliably; specifically, replace the
single runFlow: ../subflows/readPortfolio.yaml followed immediately by
assertTrue with a retry pattern (same as used earlier after minting) that calls
runFlow: ../subflows/readPortfolio.yaml up to N times with a short delay/backoff
and breaks once output.portfolio - output.portfolioBefore === 10 is true, then
perform the assertTrue on output.portfolio to fail only after exhausting
retries.
In @.maestro/src/script/hookProposals.ts:
- Around line 37-40: The spin loop that calls readContract({ address:
proposalManagerAddress, functionName: "nonces", args: [account], abi:
proposalManagerAbi }) < nextNonce in hookProposals.ts will busy-wait and spike
CPU; modify the loop to await a short sleep between iterations (e.g., await new
Promise(r => setTimeout(r, 100))) or use any provided Maestro sleep utility,
ensure the enclosing function is async, and keep the same condition using the
nonces read so you still break once nonces >= nextNonce.
- Line 49: The loop calling activity(log.address, account,
Number(BigInt(log.data))) can lose precision for values >
Number.MAX_SAFE_INTEGER; change to preserve full integer precision by passing
BigInt or a string instead of converting to Number: update the call site in the
logs loop and adjust the activity(...) signature/consumers (and any downstream
types) to accept a BigInt or string representation (e.g., BigInt(log.data) or
log.data.toString()) so large token amounts are handled without precision loss.
In @.maestro/subflows/borrowAsset.yaml:
- Around line 3-4: The existing assertion checks for amount and installments
only verify presence and numeric-ness but allow negatives and fractions; update
the assertTrue conditions that reference amount and installments so amount is a
positive integer (Number(amount) > 0 and Number.isInteger(Number(amount))) and
installments is a positive integer (Number(installments) > 0 and
Number.isInteger(Number(installments))); locate and replace the two assertTrue
expressions that use ${amount ...} and ${installments ...} to enforce these
positive-integer constraints so UI labels like "X installments of" always
receive valid whole, positive counts and amounts.
In `@server/test/docker-compose.yaml`:
- Line 4: The docker-compose uses floating :latest tags for otterscan/otterscan
and ghcr.io/ethereum/sourcify/server which harms reproducibility; update the
image entries for those services (otterscan/otterscan and
ghcr.io/ethereum/sourcify/server) to a fixed tag or immutable digest (e.g.,
specific version tag or sha256 digest) instead of :latest, ensuring both
occurrences are changed and commit the updated compose file so the test
environment uses pinned images.
In `@server/test/e2e.ts`:
- Line 94: Remove the extraneous blank line separating the two test calls so
resumeInquiry and getDocument are adjacent; open server/test/e2e.ts, locate the
sequence where resumeInquiry(...) is followed by an empty line and then
getDocument(...), and delete that blank line so the calls sit on consecutive
lines in accordance with the compact style guideline.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/loans/AmountSelector.tsx`:
- Line 94: Change the aria-label on the AmountSelector component to use a
full-sentence i18n string instead of the terse "Amount": update the aria-label
prop (in src/components/loans/AmountSelector.tsx where aria-label={t("Amount")}
is used) to call t with a full-sentence key/value (e.g., t("Select the loan
amount in dollars")) or the repository's agreed i18n key that maps to a full
sentence so screen readers get contextual information; ensure the exact key
matches existing i18n files or add the new sentence to the translations.
In `@src/utils/e2e.ts`:
- Around line 62-77: The wallet_getCallsStatus RPC handler throws a vague
Error("bad"); change it to use the consistent error wording used elsewhere
(e.g., throw new Error("bad params")) so callers get uniform messages; update
the error thrown in the case "wallet_getCallsStatus" (where params, isHex and id
are validated) to the same phrasing used by other handlers like "bad params".
dce1252 to
b1b5b78
Compare
PR SummaryMedium Risk Overview Enhances Maestro blockchain scripting by adding Improves local/e2e infrastructure: the mobile e2e Written by Cursor Bugbot for commit 09ea4d9. This will update automatically on new commits. Configure here. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
1 participant
Summary by CodeRabbit
Accessibility
New Features
Chores / Tests