feat: use magicsvm#93
Open
Dodecahedr0x wants to merge 14 commits into
Open
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 58 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds shared test infrastructure for MagicSVM, a Solana transaction simulation library, and migrates two example test suites from live RPC connections to in-memory test execution. A new test-utils package exports transaction conversion helpers, the test runner is updated to invoke MagicSVM tests, and both dummy-token-transfer and pinocchio-counter tests are rewritten to use simulated execution. ChangesMagicSVM test infrastructure and migrations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pinocchio-counter/tests/web3js/pinocchio-counter.test.ts`:
- Around line 110-112: The test throws an error that interpolates the metadata
object directly; update the initialize failure handling in the test so that when
res is an instance of FailedTransactionMetadata you include human-readable
failure details (e.g., call res.meta().prettyLogs() or res.err()) in the thrown
message instead of interpolating res; locate the failing block that checks "if
(res instanceof FailedTransactionMetadata)" and replace the interpolation with
the preferred readable method such as res.meta().prettyLogs().
In `@pinocchio-ephemeral-permission-counter/src/processor.rs`:
- Around line 20-23: The hard-coded rent calculation in rent_for(size: u64) is
brittle; replace it by using the runtime/Solana SDK rent API to compute the
exact minimum balance for rent exemption based on the account size instead of
"(size + 60) * 32". Update the rent_for function (and its uses in
process_initialize_counter and wherever else referenced) to call the SDK rent
getter (e.g., obtain the Rent sysvar and use
get_minimum_balance_for_rent_exemption / minimum_balance with the correct
account data length) so the PDA funding matches the cluster rent parameters and
the actual permission account layout.
- Around line 167-201: The code currently allows any caller to be set as the
permission authority because process_create_permission uses counter_info as the
permissioned_account and never requires authority_info to be a signer; to fix
it, require authority_info.is_signer() before creating the CPI and set
CreateEphemeralPermission.permissioned_account to authority_info (keep payer as
counter_info if intended), ensuring the actual authority signs the instruction;
update process_create_permission to validate authority_info.is_signer() and pass
authority_info as the permissioned_account in the CreateEphemeralPermission
struct so the declared authority must sign.
In
`@pinocchio-ephemeral-permission-counter/tests/pinocchio-ephemeral-permission-counter.test.ts`:
- Around line 61-80: The test currently logs sensitive bearer-token URLs
(teeUserUrl / teeUserWsUrl) and connection endpoints
(connectionEphemeralRollup.rpcEndpoint, unauthorizedConnection.rpcEndpoint,
connectionBaseLayer.rpcEndpoint) which may include ?token=...; remove or
sanitize those logs by either not printing the full URL or stripping/replacing
the token query parameter before logging (e.g., parse the URL and log origin +
pathname or replace token value with [REDACTED]); update all console.log calls
in the test file that reference teeUserUrl, teeUserWsUrl,
connectionEphemeralRollup.rpcEndpoint, unauthorizedConnection.rpcEndpoint, and
connectionBaseLayer.rpcEndpoint to use the sanitized value or omit them
entirely.
- Around line 126-138: The beforeAll setup unconditionally POSTs getIdentity to
teeUrl which fails for non-TEE endpoints; modify the beforeAll in the test file
to skip the fetch when the current endpoint is non-TEE (use the existing
EPHEMERAL_PROVIDER_ENDPOINT/teeUrl value to detect non-TEE) or at minimum wrap
the fetch in a try/catch and only set validator = new PublicKey(...) when a
valid data.result.identity is returned; update the beforeAll logic (referencing
beforeAll, teeUrl, validator, and PublicKey) so tests use the already-present
fallback validator for non-TEE endpoints instead of failing the setup.
- Around line 36-108: The test's describe callback currently performs async
bootstrap (reading KEYPAIR, calling getAuthToken, building teeUrl/teeUserUrl,
connectionEphemeralRollup, unauthorizedConnection, etc.) at collection time;
make the describe callback synchronous and move all async work (KEYPAIR parsing,
getAuthToken calls, building authToken/teeUserUrl/teeUserWsUrl, and Connection
initialization for connectionEphemeralRollup and unauthorizedConnection) into a
beforeAll (or a helper invoked from beforeAll), leaving only synchronous
declarations in the top-level describe and assigning the results to variables
declared in the outer scope so tests can use them.
In `@spl-tokens/app/src/App.tsx`:
- Around line 734-738: scheduleBalanceFallbackRefresh currently creates timeouts
without cancelling previous timers or clearing them on unmount; update it to
debounce and clean up by using a mutable ref (e.g., balanceFallbackTimerRef) to
store the timeout id, clearTimeout(balanceFallbackTimerRef.current) before
creating a new setTimeout that calls refreshBalances(), assign the new id to the
ref, and add a useEffect cleanup that clears the ref on unmount so pending
timers won't fire after unmount; reference scheduleBalanceFallbackRefresh,
refreshBalances, BALANCE_FALLBACK_REFRESH_DELAY_MS and the new
balanceFallbackTimerRef in the fix.
In `@test-utils/package.json`:
- Around line 9-11: The test-utils package is using `@solana/kit` v6.9.0 while
consumers (e.g., pinocchio-counter) use v5.x which can cause incompatible
AccountRole enums used by transactionFromKitTransactionMessage and break
instruction account role mapping; update package.json in test-utils and all
consumer packages to align `@solana/kit` to the same major (recommend 6.9.0), and
audit/align `@solana/web3.js` major versions to ensure TransactionMessage and
related types are compatible; after bumping, run tests that exercise
transactionFromKitTransactionMessage(msg: TransactionMessage, ...) and any code
referencing AccountRole to confirm signer/writable classification remains
correct.
In `@test-utils/src/index.ts`:
- Around line 60-118: Both transactionFromWeb3Transaction and
transactionFromKitTransactionMessage duplicate the same tail that sets feePayer,
recentBlockhash, lastValidBlockHeight and performs partial signing; extract this
into a new helper finalizeTransaction(transaction, options) that sets
transaction.feePayer = new Address(options.payer.address),
transaction.recentBlockhash = options.recentBlockhash,
transaction.lastValidBlockHeight = 0n, iterates options.signers to await
transaction.partialSign(signer) and finally awaits
transaction.partialSign(options.payer), then call finalizeTransaction(...) from
both builders and remove the duplicated code.
- Around line 79-81: Remove the three leftover debug console.log calls that
print transaction, transaction.signatures, and transaction.signature from
test-utils/src/index.ts; locate the code that references the variable
transaction (and the lines with console.log(transaction),
console.log(transaction.signatures), console.log(transaction.signature)) and
delete them, or replace them with a non-verbose debug/log call behind a toggle
(e.g., use an existing logger.debug) if you need optional tracing instead of
unconditional console output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 99fbe70a-64c6-4dbd-a7ec-298a6efe1dd9
⛔ Files ignored due to path filters (6)
dummy-token-transfer/package-lock.jsonis excluded by!**/package-lock.jsondummy-token-transfer/yarn.lockis excluded by!**/yarn.lock,!**/*.lockpinocchio-counter/yarn.lockis excluded by!**/yarn.lock,!**/*.lockpinocchio-ephemeral-permission-counter/yarn.lockis excluded by!**/yarn.lock,!**/*.locktest-utils/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (37)
00-LEGACY_EXAMPLES/anchor-counter/app/src/App.tsx00-LEGACY_EXAMPLES/private-counter/app/src/App.tsx00-LEGACY_EXAMPLES/private-counter/app/src/index.cssanchor-counter/app/src/App.tsxdummy-token-transfer/Anchor.tomldummy-token-transfer/package.jsondummy-token-transfer/programs/dummy-transfer/src/lib.rsdummy-token-transfer/tests/dummy-transfer-local.tsdummy-token-transfer/tests/dummy-transfer.tsdummy-token-transfer/tsconfig.jsonpinocchio-counter/package.jsonpinocchio-counter/tests/kit/initializeKeypair.tspinocchio-counter/tests/kit/pinocchio-counter.test.tspinocchio-counter/tests/web3js/initializeKeypair.tspinocchio-counter/tests/web3js/pinocchio-counter.test.tspinocchio-ephemeral-permission-counter/.env.examplepinocchio-ephemeral-permission-counter/.gitignorepinocchio-ephemeral-permission-counter/.yarnrc.ymlpinocchio-ephemeral-permission-counter/Cargo.tomlpinocchio-ephemeral-permission-counter/LICENSEpinocchio-ephemeral-permission-counter/README.mdpinocchio-ephemeral-permission-counter/package.jsonpinocchio-ephemeral-permission-counter/src/entrypoint.rspinocchio-ephemeral-permission-counter/src/lib.rspinocchio-ephemeral-permission-counter/src/processor.rspinocchio-ephemeral-permission-counter/src/state.rspinocchio-ephemeral-permission-counter/tests/pinocchio-ephemeral-permission-counter.test.tspinocchio-ephemeral-permission-counter/tsconfig.jsonprivate-counter/app/src/App.tsxprivate-counter/app/src/index.cssspl-tokens/app/src/App.tsxspl-tokens/app/src/components/Wizard.tsxtest-locally.shtest-utils/.yarnrc.ymltest-utils/README.mdtest-utils/package.jsontest-utils/src/index.ts
💤 Files with no reviewable changes (4)
- spl-tokens/app/src/components/Wizard.tsx
- pinocchio-counter/tests/kit/initializeKeypair.ts
- pinocchio-counter/tests/web3js/initializeKeypair.ts
- dummy-token-transfer/tests/dummy-transfer-local.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pinocchio-counter/tests/kit/pinocchio-counter.test.ts`:
- Line 34: The test suite callback for describe is declared async which can
cause non-deterministic test registration; change describe("basic-test", async
() => { ... }) to a synchronous describe("basic-test", () => { ... }) and move
any asynchronous setup/teardown into lifecycle hooks (e.g., beforeAll,
beforeEach, afterAll) or into the individual it(...) callbacks so all async work
(promises, awaits) runs inside those hooks or tests rather than in the describe
callback.
In `@pinocchio-counter/tests/web3js/pinocchio-counter.test.ts`:
- Around line 336-342: The test uses an ephemeral send but passes
svm.latestBlockhash() as recentBlockhash, which can cause commit failures; in
the call that builds the transaction (transactionFromWeb3Transaction with payer:
signerFromWeb3Keypair(userKeypair)), replace recentBlockhash:
svm.latestBlockhash() with recentBlockhash: svm.latestBlockhashFor({ target:
"ephemeral" }) so the blockhash matches the ephemeral target semantics.
- Line 32: The test uses an async callback in describe (describe("basic-test",
async () => {)) — change the describe callback to synchronous and move any async
setup/teardown into beforeAll or beforeEach so async awaits occur there (update
the test bootstrap code accordingly); additionally in the "Commit counter state
on ER to Solana" transaction where send is called with { target: "ephemeral" }
but the blockhash is fetched via svm.latestBlockhash(), replace that call with
svm.latestBlockhashFor({ target: "ephemeral" }) so the recentBlockhash matches
the ephemeral target for the send call.
In `@test-locally.sh`:
- Around line 396-398: The current one-liner "cd test-utils && yarn install &&
cd .." can leave the script stuck in test-utils on failure; change it to run
pushd test-utils; yarn install || { echo "yarn install failed"; popd; exit 1; };
popd (or equivalent explicit cd .. and exit on non-zero) so the script always
returns to the original cwd and aborts if yarn install fails; update the block
replacing that exact command sequence to ensure proper error handling and cwd
restoration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73e89a50-33d4-40f6-a8c9-b8a79a06b8d4
⛔ Files ignored due to path filters (2)
pinocchio-counter/yarn.lockis excluded by!**/yarn.lock,!**/*.locktest-utils/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
dummy-token-transfer/tests/dummy-transfer.tspinocchio-counter/package.jsonpinocchio-counter/tests/kit/initializeKeypair.tspinocchio-counter/tests/kit/pinocchio-counter.test.tspinocchio-counter/tests/web3js/initializeKeypair.tspinocchio-counter/tests/web3js/pinocchio-counter.test.tstest-locally.shtest-utils/package.jsontest-utils/src/index.ts
💤 Files with no reviewable changes (2)
- pinocchio-counter/tests/web3js/initializeKeypair.ts
- pinocchio-counter/tests/kit/initializeKeypair.ts
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.
Uses MagicSVM to run tests instead of a local validator
Summary by CodeRabbit
New Features
Chores
Tests