fix: migrate integration tests from testnet to local Anvil#40
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR replaces env/testnet-driven integration tests with a self-contained Anvil-based test harness, adds Foundry integration and mock Solidity contracts (MockPerpManager, MockUSDC, MockFees, MockMarginRatios), refactors tests to use a new anvil-setup helper, and updates CI/test config and artifacts. Changes
Sequence Diagram(s)sequenceDiagram
%% Component rectangles colored with rgba(0,0,0,0.5) style not supported in sequenceDiagram nodes,
%% keep interactions clear and minimal.
actor Test
participant AnvilSetup
participant Anvil
participant Artifacts
participant WalletClient
participant PublicClient
participant MockContracts
Test->>AnvilSetup: setupAnvil()
AnvilSetup->>Anvil: spawn Anvil (fixed port)
AnvilSetup->>Anvil: await RPC readiness
AnvilSetup->>Artifacts: load compiled ABIs/bytecode
Artifacts-->>AnvilSetup: provide ABIs/bytecode
AnvilSetup->>WalletClient: create account (ANVIL_PRIVATE_KEY)
AnvilSetup->>PublicClient: init HTTP client -> Anvil RPC
AnvilSetup->>MockContracts: deploy(MockFees, MockMarginRatios, MockUSDC, MockPerpManager)
MockContracts-->>AnvilSetup: deployed addresses
AnvilSetup->>MockContracts: setupPerp(testPerpId, poolKey, ...)
AnvilSetup->>MockContracts: mint USDC to test account
AnvilSetup-->>Test: return AnvilSetup{rpcUrl, addresses, publicClient, wallet/account, context, cleanup}
Test->>Test: run integration tests using setup.*
Test->>AnvilSetup: cleanup()
AnvilSetup->>Anvil: terminate process
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/contracts/cache/solidity-files-cache.json (1)
1-1: Consider adding this cache file to.gitignore.Build cache files like
solidity-files-cache.jsonare generated artifacts that can cause merge conflicts and unnecessary repository churn. They will be regenerated byforge build. If reproducibility is needed, the compiled artifacts inout/are sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/contracts/cache/solidity-files-cache.json` at line 1, Remove the tracked build cache file tests/contracts/cache/solidity-files-cache.json from source control and add it to .gitignore; specifically, add an entry for solidity-files-cache.json (or the tests/contracts/cache/ directory) to .gitignore, run git rm --cached tests/contracts/cache/solidity-files-cache.json (or git rm --cached -r tests/contracts/cache if you ignore the whole dir), and commit the updated .gitignore and removal so future forge builds regenerate the file locally without causing merge churn.src/__tests__/helpers/anvil-setup.ts (2)
279-309: Type casts (as any) may hide type mismatches.The casts on lines 281, 306, and 307 suggest the
WalletClientandPublicClienttypes from the local Anvil chain definition don't align with whatPerpCityContextexpects. This works but loses type safety.Consider narrowing the types or adjusting
PerpCityContextto accept the broader viem client types, which would provide better compile-time checking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/helpers/anvil-setup.ts` around lines 279 - 309, The test is silencing type mismatches by using "as any" on walletClient and publicClient when constructing PerpCityContext and returning them; update types instead: either cast walletClient/publicClient to the exact WalletClient/PublicClient interfaces expected by PerpCityContext (or wrap/transform the viem clients into the expected adapter), or broaden PerpCityContext's constructor types to accept the viem client types used in tests (e.g., change PerpCityContext constructor parameter typings to accept viem WalletClient/PublicClient unions). Locate the PerpCityContext constructor signature and the variables walletClient/publicClient in this file and replace "as any" with proper typed references or adjust PerpCityContext types so the test compiles without using "as any".
76-137: Hardcoded port may cause parallel test conflicts.The fixed port
8546could cause issues if multiple integration test suites run concurrently (e.g., in CI with parallelism). Consider using port 0 for dynamic allocation or accepting a port parameter.💡 Optional: Dynamic port allocation
-function startAnvil(port: number): Promise<{ process: ChildProcess; rpcUrl: string }> { +function startAnvil(port: number = 0): Promise<{ process: ChildProcess; rpcUrl: string }> { return new Promise((resolve, reject) => { const anvilProcess = spawn( "anvil", - ["--chain-id", "31337", "--block-time", "1", "--port", String(port)], + ["--chain-id", "31337", "--block-time", "1", ...(port ? ["--port", String(port)] : [])],Then parse the actual port from Anvil's output (e.g., "Listening on 127.0.0.1:XXXXX").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/helpers/anvil-setup.ts` around lines 76 - 137, startAnvil currently assumes a fixed port; change it to support dynamic allocation by allowing port to be optional or 0 and, when 0 is used, spawn Anvil with "--port", "0" and parse the "Listening on 127.0.0.1:XXXXX" line from anvilProcess.stdout/stderr to extract the actual port number before resolving; update the resolve payload (process and rpcUrl) to use the parsed port, keep existing timeout and error handling, and ensure the function signature (startAnvil) and variables (anvilProcess, rpcUrl, timeout, resolved) are updated accordingly so callers can pass 0 or omit the port to avoid conflicts in parallel tests.tests/contracts/MockPerpManager.sol (1)
384-408: Note:wasMakeris hardcoded tofalsein PositionClosed event.Line 400 always emits
wasMaker: falseregardless of actual position type. If tests need to verify maker position closures, this would need adjustment.💡 Suggested fix to preserve maker status
+ bool isMaker = pos.makerDetails.liquidity > 0; + emit PositionClosed( pos.perpId, _sqrtPrices[pos.perpId], 1000, // longOI 1000, // shortOI params.posId, - false, // wasMaker + isMaker, // wasMaker quote.wasLiquidated,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/contracts/MockPerpManager.sol` around lines 384 - 408, The PositionClosed event currently hardcodes wasMaker as false in closePosition; replace that literal with the actual maker flag from the Position pulled from _positions (e.g., use pos.isMaker or pos.wasMaker depending on the struct field name) so the emitted wasMaker reflects the stored position state, and if the Position struct lacks such a field add and populate it when positions are created/updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/helpers/anvil-setup.ts`:
- Around line 279-309: The test is silencing type mismatches by using "as any"
on walletClient and publicClient when constructing PerpCityContext and returning
them; update types instead: either cast walletClient/publicClient to the exact
WalletClient/PublicClient interfaces expected by PerpCityContext (or
wrap/transform the viem clients into the expected adapter), or broaden
PerpCityContext's constructor types to accept the viem client types used in
tests (e.g., change PerpCityContext constructor parameter typings to accept viem
WalletClient/PublicClient unions). Locate the PerpCityContext constructor
signature and the variables walletClient/publicClient in this file and replace
"as any" with proper typed references or adjust PerpCityContext types so the
test compiles without using "as any".
- Around line 76-137: startAnvil currently assumes a fixed port; change it to
support dynamic allocation by allowing port to be optional or 0 and, when 0 is
used, spawn Anvil with "--port", "0" and parse the "Listening on
127.0.0.1:XXXXX" line from anvilProcess.stdout/stderr to extract the actual port
number before resolving; update the resolve payload (process and rpcUrl) to use
the parsed port, keep existing timeout and error handling, and ensure the
function signature (startAnvil) and variables (anvilProcess, rpcUrl, timeout,
resolved) are updated accordingly so callers can pass 0 or omit the port to
avoid conflicts in parallel tests.
In `@tests/contracts/cache/solidity-files-cache.json`:
- Line 1: Remove the tracked build cache file
tests/contracts/cache/solidity-files-cache.json from source control and add it
to .gitignore; specifically, add an entry for solidity-files-cache.json (or the
tests/contracts/cache/ directory) to .gitignore, run git rm --cached
tests/contracts/cache/solidity-files-cache.json (or git rm --cached -r
tests/contracts/cache if you ignore the whole dir), and commit the updated
.gitignore and removal so future forge builds regenerate the file locally
without causing merge churn.
In `@tests/contracts/MockPerpManager.sol`:
- Around line 384-408: The PositionClosed event currently hardcodes wasMaker as
false in closePosition; replace that literal with the actual maker flag from the
Position pulled from _positions (e.g., use pos.isMaker or pos.wasMaker depending
on the struct field name) so the emitted wasMaker reflects the stored position
state, and if the Position struct lacks such a field add and populate it when
positions are created/updated.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/ci.ymlsrc/__tests__/e2e-setup.tssrc/__tests__/helpers/anvil-setup.tssrc/__tests__/helpers/testnet-config.tssrc/__tests__/integration/approval.test.tssrc/__tests__/integration/context.test.tssrc/__tests__/integration/trading.test.tstests/contracts/MockFees.soltests/contracts/MockMarginRatios.soltests/contracts/MockPerpManager.soltests/contracts/MockUSDC.soltests/contracts/cache/solidity-files-cache.jsontests/contracts/foundry.tomltests/contracts/out/MockFees.sol/MockFees.jsontests/contracts/out/MockMarginRatios.sol/MockMarginRatios.jsontests/contracts/out/MockPerpManager.sol/MockPerpManager.jsontests/contracts/out/MockUSDC.sol/MockUSDC.jsontests/contracts/out/build-info/525d107cfebc343c.jsonvitest.e2e.config.ts
💤 Files with no reviewable changes (2)
- src/tests/e2e-setup.ts
- src/tests/helpers/testnet-config.ts
Replace Base Sepolia testnet dependency with local Anvil (Foundry) for all integration tests, eliminating the need for repo secrets (RPC_URL, PRIVATE_KEY, etc.) in CI. - Add mock Solidity contracts (MockPerpManager, MockUSDC, MockFees, MockMarginRatios) compiled with Forge - Add anvil-setup.ts helper that starts Anvil, deploys mocks, and creates a test PerpCityContext - Rewrite approval, context, and trading integration tests to use setupAnvil() instead of testnet config - Remove e2e-setup.ts and testnet-config.ts (no longer needed) - Update CI workflow to install Foundry instead of injecting secrets - Remove all env var dependencies from test files
koko1123
force-pushed
the
koko/fix-integration-tests
branch
from
9c47f54 to
a5469f6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
tests/contracts/MockPerpManager.sol (1)
305-313:createPerpdoes not populate the config mapping.The
createPerpfunction sets_perpExistsand_sqrtPricesbut doesn't populate_configs. Calls tocfgs(perpId)for perps created viacreatePerpwill return zero/empty values. This may be intentional if tests usesetupPerpfor full configuration, but could cause confusion.Consider adding a note or populating minimal config
If this is intentional, consider adding a comment clarifying that
createPerpis for minimal perp creation andsetupPerpshould be used for full configuration:/// `@notice` Create a new perp market + /// `@dev` Only sets existence and price. Use setupPerp() for full config. function createPerp(CreatePerpParams calldata params) external returns (bytes32 perpId) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/contracts/MockPerpManager.sol` around lines 305 - 313, The createPerp function currently sets _perpExists and _sqrtPrices but never initializes the _configs mapping, so calls to cfgs(perpId) return empty values; either document that createPerp is intentionally minimal and advise using setupPerp for full configuration, or populate a minimal config entry inside createPerp (e.g., set _configs[perpId] with sensible defaults) so cfgs(perpId) returns valid data; update createPerp (and mention symbols _configs, cfgs, createPerp, setupPerp, _perpExists, _sqrtPrices) accordingly.src/__tests__/helpers/anvil-setup.ts (2)
167-170: Fixed port may cause issues with parallel test execution.Using a hardcoded port (8546) will fail if multiple test suites run concurrently in CI. Consider using port 0 to let the OS assign an available port, or implement port discovery.
Consider dynamic port allocation
export async function setupAnvil(): Promise<AnvilSetup> { // Start Anvil - const port = 8546; + // Use a random port to allow parallel test execution + const port = 8546 + Math.floor(Math.random() * 1000); const { process: anvilProcess, rpcUrl } = await startAnvil(port);Alternatively, use port 0 and parse the actual port from Anvil's output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/helpers/anvil-setup.ts` around lines 167 - 170, The test helper setupAnvil currently hardcodes port = 8546 which breaks parallel runs; change setupAnvil to pass a dynamic port (use 0 or a discovered free port) into startAnvil and then capture the actual assigned port/rpcUrl from startAnvil's output (or parse Anvil's stdout) so tests use the real port; update the local variable port and any callers that rely on it (symbols: setupAnvil, startAnvil, port, rpcUrl) to use the dynamically assigned value instead of the fixed 8546.
279-289: Type casts reduce type safety.The
walletClient as anycast (and similar casts at lines 306-307) bypasses TypeScript's type checking. This is likely needed due to viem's strict generic types.Consider using more specific type assertions or adjusting the
PerpCityContextconfig types to accept the concrete client types from this setup, which would preserve type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/helpers/anvil-setup.ts` around lines 279 - 289, The tests use broad "as any" casts (e.g., walletClient as any and similar casts) which weaken type safety; update the code to either cast to the precise expected types or make PerpCityContext accept the concrete viem client types: replace "walletClient as any" with a narrow assertion to the actual WalletClient type (or the interface PerpCityContext expects), and do the same for the other casts referenced, or adjust PerpCityContext's config/generics to accept the concrete viem client types so you can pass walletClient, rpc client, etc., without using any; locate the casts around PerpCityContext construction and the other casts at the noted lines and change them to specific type assertions or update the PerpCityContext config types accordingly.src/__tests__/integration/context.test.ts (2)
197-210: Strengthen assertions with exact mock-backed valuesGiven deterministic seeded data in
beforeAll, these checks can be exact (not only type checks) to catch regressions earlier.Suggested tightening
expect(rawData).toBeDefined(); -expect(rawData.perpId).toBeTypeOf("string"); +expect(rawData.perpId).toBe(setup.testPerpId); expect(rawData.positionId).toBe(TEST_POSITION_ID); expect(rawData.margin).toBeTypeOf("number"); -expect(rawData.entryPerpDelta).toBeTypeOf("bigint"); -expect(rawData.entryUsdDelta).toBeTypeOf("bigint"); +expect(rawData.entryPerpDelta).toBe(1000_000000n); +expect(rawData.entryUsdDelta).toBe(-1000_000000n); expect(rawData.marginRatios).toBeDefined(); -expect(rawData.marginRatios.min).toBeTypeOf("number"); -expect(rawData.marginRatios.max).toBeTypeOf("number"); -expect(rawData.marginRatios.liq).toBeTypeOf("number"); +expect(rawData.marginRatios).toEqual({ min: 100000, max: 500000, liq: 50000 });Based on learnings: “prefer exact value assertions over range/type assertions for intentionally hardcoded placeholders to act as tripwires.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/integration/context.test.ts` around lines 197 - 210, Update the "should fetch raw position data for valid position" test to assert exact, deterministic values (from the seeded data set up in beforeAll) instead of loose type checks: call context.getPositionRawData(TEST_POSITION_ID) as before but replace expect(...).toBeTypeOf(...) for perpId, margin, entryPerpDelta, entryUsdDelta and each marginRatios field with exact equality assertions matching the known seeded constants/fixtures (use the same TEST_POSITION_ID and the seeded fixture values used in beforeAll); keep the positionId equality check and ensure the expected numeric vs bigint types match the seeded values when asserting exact equality.
171-179: Avoid time-ratio assertions for cache behaviorThis test depends on runtime timing (
duration2 < duration1 * 0.5), which is noisy in CI and tends to become flaky. Prefer deterministic cache-state/call-path assertions.Suggested direction
- const startTime1 = Date.now(); - await context.getPerpConfig(setup.testPerpId); - const duration1 = Date.now() - startTime1; - - const startTime2 = Date.now(); - await context.getPerpConfig(setup.testPerpId); - const duration2 = Date.now() - startTime2; - - expect(duration2).toBeLessThan(duration1 * 0.5); + expect((context as any).configCache.has(setup.testPerpId)).toBe(false); + const config1 = await context.getPerpConfig(setup.testPerpId); + expect((context as any).configCache.has(setup.testPerpId)).toBe(true); + const config2 = await context.getPerpConfig(setup.testPerpId); + expect(config2).toEqual(config1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/integration/context.test.ts` around lines 171 - 179, Replace the flaky timing assertion with a deterministic cache-call assertion: instead of measuring durations, spy or mock the underlying data-fetch method that getPerpConfig(setup.testPerpId) uses (e.g., the repository method or private loader like fetchPerpConfig, _loadPerpConfig, or perpConfigRepo.get) and assert that after calling context.getPerpConfig twice the loader was invoked only once; alternatively assert that the internal cache (perpConfigCache or similar) contains setup.testPerpId after the first call and that the second call returns the same cached object instance. Use jest.spyOn or a mock for the loader to make the assertion deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 82-84: The workflow step "Install Foundry" uses the mutable action
reference foundry-rs/foundry-toolchain@v1; pin this to a specific commit SHA
instead of `@v1` and add an explicit Foundry version via the action inputs (e.g.,
add a with: version: "<fixed-foundry-version>") so the step uses
foundry-rs/foundry-toolchain@<commit-sha> and a concrete version string to
prevent CI drift.
In `@src/__tests__/integration/context.test.ts`:
- Around line 142-145: The test is wrapping the awaited call in an async
function passed to expect, but Vitest's .rejects matcher must receive the
Promise itself; replace the wrapper so you call
expect(context.getOpenPositionData(setup.testPerpId, nonExistentPositionId,
true, false)).rejects.toThrow() (and apply the same change to the other similar
cases at the later occurrences), removing the outer async ()=> wrapper and the
inner await so the Promise is passed directly to expect.
---
Nitpick comments:
In `@src/__tests__/helpers/anvil-setup.ts`:
- Around line 167-170: The test helper setupAnvil currently hardcodes port =
8546 which breaks parallel runs; change setupAnvil to pass a dynamic port (use 0
or a discovered free port) into startAnvil and then capture the actual assigned
port/rpcUrl from startAnvil's output (or parse Anvil's stdout) so tests use the
real port; update the local variable port and any callers that rely on it
(symbols: setupAnvil, startAnvil, port, rpcUrl) to use the dynamically assigned
value instead of the fixed 8546.
- Around line 279-289: The tests use broad "as any" casts (e.g., walletClient as
any and similar casts) which weaken type safety; update the code to either cast
to the precise expected types or make PerpCityContext accept the concrete viem
client types: replace "walletClient as any" with a narrow assertion to the
actual WalletClient type (or the interface PerpCityContext expects), and do the
same for the other casts referenced, or adjust PerpCityContext's config/generics
to accept the concrete viem client types so you can pass walletClient, rpc
client, etc., without using any; locate the casts around PerpCityContext
construction and the other casts at the noted lines and change them to specific
type assertions or update the PerpCityContext config types accordingly.
In `@src/__tests__/integration/context.test.ts`:
- Around line 197-210: Update the "should fetch raw position data for valid
position" test to assert exact, deterministic values (from the seeded data set
up in beforeAll) instead of loose type checks: call
context.getPositionRawData(TEST_POSITION_ID) as before but replace
expect(...).toBeTypeOf(...) for perpId, margin, entryPerpDelta, entryUsdDelta
and each marginRatios field with exact equality assertions matching the known
seeded constants/fixtures (use the same TEST_POSITION_ID and the seeded fixture
values used in beforeAll); keep the positionId equality check and ensure the
expected numeric vs bigint types match the seeded values when asserting exact
equality.
- Around line 171-179: Replace the flaky timing assertion with a deterministic
cache-call assertion: instead of measuring durations, spy or mock the underlying
data-fetch method that getPerpConfig(setup.testPerpId) uses (e.g., the
repository method or private loader like fetchPerpConfig, _loadPerpConfig, or
perpConfigRepo.get) and assert that after calling context.getPerpConfig twice
the loader was invoked only once; alternatively assert that the internal cache
(perpConfigCache or similar) contains setup.testPerpId after the first call and
that the second call returns the same cached object instance. Use jest.spyOn or
a mock for the loader to make the assertion deterministic.
In `@tests/contracts/MockPerpManager.sol`:
- Around line 305-313: The createPerp function currently sets _perpExists and
_sqrtPrices but never initializes the _configs mapping, so calls to cfgs(perpId)
return empty values; either document that createPerp is intentionally minimal
and advise using setupPerp for full configuration, or populate a minimal config
entry inside createPerp (e.g., set _configs[perpId] with sensible defaults) so
cfgs(perpId) returns valid data; update createPerp (and mention symbols
_configs, cfgs, createPerp, setupPerp, _perpExists, _sqrtPrices) accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/ci.ymlsrc/__tests__/e2e-setup.tssrc/__tests__/helpers/anvil-setup.tssrc/__tests__/helpers/testnet-config.tssrc/__tests__/integration/approval.test.tssrc/__tests__/integration/context.test.tssrc/__tests__/integration/trading.test.tstests/contracts/MockFees.soltests/contracts/MockMarginRatios.soltests/contracts/MockPerpManager.soltests/contracts/MockUSDC.soltests/contracts/cache/solidity-files-cache.jsontests/contracts/foundry.tomltests/contracts/out/MockFees.sol/MockFees.jsontests/contracts/out/MockMarginRatios.sol/MockMarginRatios.jsontests/contracts/out/MockPerpManager.sol/MockPerpManager.jsontests/contracts/out/MockUSDC.sol/MockUSDC.jsontests/contracts/out/build-info/525d107cfebc343c.jsonvitest.e2e.config.ts
💤 Files with no reviewable changes (2)
- src/tests/helpers/testnet-config.ts
- src/tests/e2e-setup.ts
✅ Files skipped from review due to trivial changes (1)
- tests/contracts/cache/solidity-files-cache.json
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/contracts/out/build-info/525d107cfebc343c.json
- vitest.e2e.config.ts
- tests/contracts/out/MockFees.sol/MockFees.json
- tests/contracts/MockFees.sol
- tests/contracts/foundry.toml
Comment on lines
+82
to
+84
| - name: Install Foundry | ||
| uses: foundry-rs/foundry-toolchain@v1 | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file exists and examine lines around 82-84
cat -n .github/workflows/ci.yml | sed -n '75,95p'Repository: StrobeLabs/perpcity-sdk
Length of output: 641
🏁 Script executed:
# Now run the verification script from the review to see current state
#!/bin/bash
set -euo pipefail
echo "Foundry action reference:"
rg -n 'uses:\s*foundry-rs/foundry-toolchain@' .github/workflows/ci.yml -C2
echo
echo "Is the Foundry action pinned to a full SHA? (expect no match before fix)"
rg -nP 'uses:\s*foundry-rs/foundry-toolchain@[0-9a-f]{40}\b' .github/workflows/ci.yml || true
echo
echo "Is an explicit Foundry version configured near the install step?"
rg -n 'name:\s*Install Foundry|uses:\s*foundry-rs/foundry-toolchain|^\s+version:\s*' .github/workflows/ci.yml -C1Repository: StrobeLabs/perpcity-sdk
Length of output: 584
Pin Foundry action to commit SHA and set explicit Foundry version to avoid CI drift.
Line 83 uses a mutable tag (@v1) without an explicit Foundry version. Pin the action to a commit SHA and configure a fixed version to maintain reproducible builds.
Suggested fix
- name: Install Foundry
- uses: foundry-rs/foundry-toolchain@v1
+ uses: foundry-rs/foundry-toolchain@<FULL_40_CHAR_COMMIT_SHA>
+ with:
+ version: <PINNED_FOUNDRY_VERSION>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 82 - 84, The workflow step "Install
Foundry" uses the mutable action reference foundry-rs/foundry-toolchain@v1; pin
this to a specific commit SHA instead of `@v1` and add an explicit Foundry version
via the action inputs (e.g., add a with: version: "<fixed-foundry-version>") so
the step uses foundry-rs/foundry-toolchain@<commit-sha> and a concrete version
string to prevent CI drift.
Comment on lines
142
to
+145
| await expect(async () => { | ||
| await context.getOpenPositionData( | ||
| config.testPerpId as `0x${string}`, | ||
| nonExistentPositionId, | ||
| true, | ||
| false | ||
| ); | ||
| await context.getOpenPositionData(setup.testPerpId, nonExistentPositionId, true, false); | ||
| }).rejects.toThrow(); | ||
| }, 30000); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vitest .rejects toThrow documentation Promise async function
💡 Result:
In Vitest, use .rejects to assert on a Promise rejection (including an async function that throws), and await the assertion because the matcher becomes async. [1]
Promise / async function patterns
import { expect, test } from 'vitest'
async function fails() {
throw new Error('no id')
}
test('rejects with message', async () => {
await expect(fails()).rejects.toThrow('no id')
})You can also assert directly on an explicit rejected promise:
test('rejects toThrow', async () => {
const p = Promise.reject(new Error('Test'))
await expect(p).rejects.toThrowError()
})Notes:
- For sync exceptions, use
expect(() => fn()).toThrow(...)(wrap in a function). [1] - For async exceptions, use
.rejects(it unwraps the promise rejection). [1]
Remove async function wrapper from .rejects chain
In Vitest, .rejects should target the Promise directly, not an async function wrapper. Pass the async call to expect() without wrapping it in a function.
Suggested fix
- await expect(async () => {
- await context.getOpenPositionData(setup.testPerpId, nonExistentPositionId, true, false);
- }).rejects.toThrow();
+ await expect(
+ context.getOpenPositionData(setup.testPerpId, nonExistentPositionId, true, false)
+ ).rejects.toThrow();
- await expect(async () => {
- await context.getPerpConfig(invalidPerpId);
- }).rejects.toThrow();
+ await expect(context.getPerpConfig(invalidPerpId)).rejects.toThrow();
- await expect(async () => {
- await context.getPositionRawData(nonExistentPositionId);
- }).rejects.toThrow();
+ await expect(context.getPositionRawData(nonExistentPositionId)).rejects.toThrow();Also applies to: 152-155, 192-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/integration/context.test.ts` around lines 142 - 145, The test
is wrapping the awaited call in an async function passed to expect, but Vitest's
.rejects matcher must receive the Promise itself; replace the wrapper so you
call expect(context.getOpenPositionData(setup.testPerpId, nonExistentPositionId,
true, false)).rejects.toThrow() (and apply the same change to the other similar
cases at the later occurrences), removing the outer async ()=> wrapper and the
inner await so the Promise is passed directly to expect.
4 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
anvil-setup.tshelpere2e-setup.ts,testnet-config.ts, and all env var / repo secret dependencies from CITest plan
pnpm lintpassespnpm testpasses (187 unit tests)pnpm test:integrationpasses (38 integration tests against local Anvil)Summary by CodeRabbit
Tests
Chores