feat: improved notifications + session key renewal + fix vrf example#92
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughThe PR converts transaction feedback to structured objects with optional explorer URLs and extends Alert to render clickable external links. Session-keys gains session expiry tracking, temp keypair rotation, and sessionTokenV2 flows. A new pinocchio-private-counter program is added (with state, processor, entrypoint), roll-dice IDLs/programs/tests are updated to bundled IDLs and client_seed callbacks, and README/tests/scripts/package metadata are adjusted. ChangesTransaction Feedback & Session Management
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 |
![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: 5
🤖 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 `@00-LEGACY_EXAMPLES/anchor-counter/app/src/components/Alert.tsx`:
- Around line 58-64: The anchor rendered in the Alert component when href is
present should include an aria-label to inform screen reader users that the link
opens in a new tab; update the <a> element in the Alert component (the block
handling href) to add an aria-label that combines the visible message and a
clear hint like "(opens in a new tab)" (e.g., use aria-label={`${message} (opens
in a new tab)`}) so the link text and behavior are announced together.
In `@00-LEGACY_EXAMPLES/private-counter/app/src/App.tsx`:
- Around line 394-410: The code that builds explorerUrl inside the error handler
can mask the original transaction error if ensureAuthToken() throws; wrap the
call to ensureAuthToken() in a try-catch inside the block that checks ephemeral
and signature (the same place where explorerUrl is assigned), so any error from
ensureAuthToken() is caught and does not overwrite or change the original
transaction error; if token retrieval fails, proceed without the token (i.e.,
leave explorerUrl undefined or build the URL without the token) and optionally
log the token retrieval error, then call setTransactionError({ message:
`Transaction failed: ${error}`, explorerUrl }) as before.
In `@00-LEGACY_EXAMPLES/session-keys/app/src/App.tsx`:
- Around line 150-159: The deriveTempKeypair implementation uses insecure
XOR-based seed construction; replace it with a proper KDF (e.g., HKDF via Web
Crypto subtle APIs) so the 32-byte seed is derived cryptographically from the
public key and nonce, make deriveTempKeypair async and update call sites to
await it, and keep returning Keypair.fromSeed(seed) (where seed is the 32-byte
output of the HKDF) so callers using deriveTempKeypair and Keypair.fromSeed
continue to work correctly.
In `@00-LEGACY_EXAMPLES/session-keys/app/src/components/Alert.tsx`:
- Around line 58-64: The anchor returned when href is present lacks an
aria-label to announce that the link opens in a new tab; update the JSX in the
Alert component (the branch that returns <a href={href} ...
style={containerStyle}>) to include a descriptive aria-label that mentions the
link target and that it opens in a new tab (e.g., use the message or a default
text plus "opens in a new tab") so screen readers are informed; ensure you still
keep target="_blank" and rel="noopener noreferrer".
In `@session-keys/app/src/App.tsx`:
- Around line 539-552: The two branches of the if (sessionTokenPDA.current)
duplicate the success handling; keep only the branch-specific action
setSessionTokenExists(true) in the else, then move the shared logic
(setSessionExpiresAt(validUntilBN.toNumber()), compute totalMs from
performance.now() - txStart, and setTransactionSuccess({ message: `[Base]
Session created in ${totalMs}ms`, explorerUrl: baseExplorerUrl(signature) })) to
execute once after the conditional. Update references to validUntilBN, txStart,
signature, baseExplorerUrl, setSessionExpiresAt, and setTransactionSuccess
accordingly so behavior is unchanged.
🪄 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: 7055c5b5-187c-4b0c-9eb2-93df8cb8f515
📒 Files selected for processing (10)
00-LEGACY_EXAMPLES/anchor-counter/app/src/App.tsx00-LEGACY_EXAMPLES/anchor-counter/app/src/components/Alert.tsx00-LEGACY_EXAMPLES/private-counter/app/src/App.tsx00-LEGACY_EXAMPLES/session-keys/app/src/App.tsx00-LEGACY_EXAMPLES/session-keys/app/src/components/Alert.tsxanchor-counter/app/src/App.tsxanchor-counter/app/src/components/Alert.tsxprivate-counter/app/src/App.tsxsession-keys/app/src/App.tsxsession-keys/app/src/components/Alert.tsx
Comment on lines
+58
to
+64
| if (href) { | ||
| return ( | ||
| <a href={href} target="_blank" rel="noopener noreferrer" style={containerStyle}> | ||
| {message} <span style={{ marginLeft: 4 }}>↗</span> | ||
| </a> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding aria-label for screen reader users.
The external link opens in a new tab but doesn't announce this to screen readers. Adding an aria-label would improve accessibility.
♿ Optional accessibility enhancement
if (href) {
return (
- <a href={href} target="_blank" rel="noopener noreferrer" style={containerStyle}>
+ <a
+ href={href}
+ target="_blank"
+ rel="noopener noreferrer"
+ aria-label={`${message} (opens in new tab)`}
+ style={containerStyle}
+ >
{message} <span style={{ marginLeft: 4 }}>↗</span>
</a>
);
}📝 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.
Suggested change
| if (href) { | |
| return ( | |
| <a href={href} target="_blank" rel="noopener noreferrer" style={containerStyle}> | |
| {message} <span style={{ marginLeft: 4 }}>↗</span> | |
| </a> | |
| ); | |
| } | |
| if (href) { | |
| return ( | |
| <a | |
| href={href} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| aria-label={`${message} (opens in new tab)`} | |
| style={containerStyle} | |
| > | |
| {message} <span style={{ marginLeft: 4 }}>↗</span> | |
| </a> | |
| ); | |
| } |
🤖 Prompt for 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.
In `@00-LEGACY_EXAMPLES/anchor-counter/app/src/components/Alert.tsx` around lines
58 - 64, The anchor rendered in the Alert component when href is present should
include an aria-label to inform screen reader users that the link opens in a new
tab; update the <a> element in the Alert component (the block handling href) to
add an aria-label that combines the visible message and a clear hint like
"(opens in a new tab)" (e.g., use aria-label={`${message} (opens in a new
tab)`}) so the link text and behavior are announced together.
Comment on lines
+394
to
+410
| // If the tx made it on-chain (signature was returned by sendRawTransaction), | ||
| // attach an explorer link so the user can inspect the failure logs. | ||
| let explorerUrl: string | undefined; | ||
| if (signature) { | ||
| if (ephemeral) { | ||
| const token = await ensureAuthToken(); | ||
| if (token) { | ||
| explorerUrl = `https://explorer.solana.com/tx/${signature}?cluster=custom&customUrl=${encodeURIComponent(PRIVATE_ER_ENDPOINT + '?token=' + token)}`; | ||
| } | ||
| } else { | ||
| explorerUrl = `https://explorer.solana.com/tx/${signature}?cluster=devnet`; | ||
| } | ||
| } | ||
| setTransactionError({ | ||
| message: `Transaction failed: ${error}`, | ||
| explorerUrl, | ||
| }); |
Contributor
There was a problem hiding this comment.
Wrap ensureAuthToken() in try-catch to prevent masking the original error.
If ensureAuthToken() throws in the catch block, the original transaction error will be masked and the user won't see the failure message.
🛡️ Proposed fix to handle potential ensureAuthToken failure
let explorerUrl: string | undefined;
if (signature) {
if (ephemeral) {
- const token = await ensureAuthToken();
- if (token) {
- explorerUrl = `https://explorer.solana.com/tx/${signature}?cluster=custom&customUrl=${encodeURIComponent(PRIVATE_ER_ENDPOINT + '?token=' + token)}`;
+ try {
+ const token = await ensureAuthToken();
+ if (token) {
+ explorerUrl = `https://explorer.solana.com/tx/${signature}?cluster=custom&customUrl=${encodeURIComponent(PRIVATE_ER_ENDPOINT + '?token=' + token)}`;
+ }
+ } catch (tokenErr) {
+ console.error("Failed to get auth token for explorer URL:", tokenErr);
}
} else {
explorerUrl = `https://explorer.solana.com/tx/${signature}?cluster=devnet`;📝 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.
Suggested change
| // If the tx made it on-chain (signature was returned by sendRawTransaction), | |
| // attach an explorer link so the user can inspect the failure logs. | |
| let explorerUrl: string | undefined; | |
| if (signature) { | |
| if (ephemeral) { | |
| const token = await ensureAuthToken(); | |
| if (token) { | |
| explorerUrl = `https://explorer.solana.com/tx/${signature}?cluster=custom&customUrl=${encodeURIComponent(PRIVATE_ER_ENDPOINT + '?token=' + token)}`; | |
| } | |
| } else { | |
| explorerUrl = `https://explorer.solana.com/tx/${signature}?cluster=devnet`; | |
| } | |
| } | |
| setTransactionError({ | |
| message: `Transaction failed: ${error}`, | |
| explorerUrl, | |
| }); | |
| // If the tx made it on-chain (signature was returned by sendRawTransaction), | |
| // attach an explorer link so the user can inspect the failure logs. | |
| let explorerUrl: string | undefined; | |
| if (signature) { | |
| if (ephemeral) { | |
| try { | |
| const token = await ensureAuthToken(); | |
| if (token) { | |
| explorerUrl = `https://explorer.solana.com/tx/${signature}?cluster=custom&customUrl=${encodeURIComponent(PRIVATE_ER_ENDPOINT + '?token=' + token)}`; | |
| } | |
| } catch (tokenErr) { | |
| console.error("Failed to get auth token for explorer URL:", tokenErr); | |
| } | |
| } else { | |
| explorerUrl = `https://explorer.solana.com/tx/${signature}?cluster=devnet`; | |
| } | |
| } | |
| setTransactionError({ | |
| message: `Transaction failed: ${error}`, | |
| explorerUrl, | |
| }); |
🤖 Prompt for 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.
In `@00-LEGACY_EXAMPLES/private-counter/app/src/App.tsx` around lines 394 - 410,
The code that builds explorerUrl inside the error handler can mask the original
transaction error if ensureAuthToken() throws; wrap the call to
ensureAuthToken() in a try-catch inside the block that checks ephemeral and
signature (the same place where explorerUrl is assigned), so any error from
ensureAuthToken() is caught and does not overwrite or change the original
transaction error; if token retrieval fails, proceed without the token (i.e.,
leave explorerUrl undefined or build the URL without the token) and optionally
log the token retrieval error, then call setTransactionError({ message:
`Transaction failed: ${error}`, explorerUrl }) as before.
Comment on lines
+150
to
+159
| const deriveTempKeypair = useCallback((pk: PublicKey, nonce: string) => { | ||
| // 32-byte seed from sha256(publicKey || nonce). Use Web Crypto for portability. | ||
| const seedBytes = new Uint8Array(32); | ||
| const src = new TextEncoder().encode(pk.toBase58() + ":" + nonce); | ||
| // Synchronous-ish: borrow first 32 bytes by hashing manually via subtle isn't sync. | ||
| // Simpler: pad/truncate the raw bytes ourselves since they don't need to be uniform. | ||
| const raw = new Uint8Array(pk.toBytes()); | ||
| for (let i = 0; i < 32; i++) seedBytes[i] = raw[i] ^ (src[i % src.length] ?? 0); | ||
| return Keypair.fromSeed(seedBytes); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Use a proper KDF instead of XOR for key derivation.
The XOR-based seed derivation is not cryptographically sound. Even for demo purposes, this weakens the security model and could lead to predictable session keys. Use a proper key derivation function like HKDF via Web Crypto API.
🔐 Proposed fix using Web Crypto subtle.deriveBits
-const deriveTempKeypair = useCallback((pk: PublicKey, nonce: string) => {
- // 32-byte seed from sha256(publicKey || nonce). Use Web Crypto for portability.
- const seedBytes = new Uint8Array(32);
- const src = new TextEncoder().encode(pk.toBase58() + ":" + nonce);
- // Synchronous-ish: borrow first 32 bytes by hashing manually via subtle isn't sync.
- // Simpler: pad/truncate the raw bytes ourselves since they don't need to be uniform.
- const raw = new Uint8Array(pk.toBytes());
- for (let i = 0; i < 32; i++) seedBytes[i] = raw[i] ^ (src[i % src.length] ?? 0);
- return Keypair.fromSeed(seedBytes);
-}, []);
+const deriveTempKeypair = useCallback(async (pk: PublicKey, nonce: string) => {
+ // Proper KDF using Web Crypto API
+ const encoder = new TextEncoder();
+ const keyMaterial = encoder.encode(pk.toBase58() + ":" + nonce);
+ const hashBuffer = await crypto.subtle.digest('SHA-256', keyMaterial);
+ const seedBytes = new Uint8Array(hashBuffer);
+ return Keypair.fromSeed(seedBytes);
+}, []);Note: This makes deriveTempKeypair async. You'll need to await it at the call sites (lines 165, 468).
🤖 Prompt for 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.
In `@00-LEGACY_EXAMPLES/session-keys/app/src/App.tsx` around lines 150 - 159, The
deriveTempKeypair implementation uses insecure XOR-based seed construction;
replace it with a proper KDF (e.g., HKDF via Web Crypto subtle APIs) so the
32-byte seed is derived cryptographically from the public key and nonce, make
deriveTempKeypair async and update call sites to await it, and keep returning
Keypair.fromSeed(seed) (where seed is the 32-byte output of the HKDF) so callers
using deriveTempKeypair and Keypair.fromSeed continue to work correctly.
Comment on lines
+58
to
+64
| if (href) { | ||
| return ( | ||
| <a href={href} target="_blank" rel="noopener noreferrer" style={containerStyle}> | ||
| {message} <span style={{ marginLeft: 4 }}>↗</span> | ||
| </a> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding aria-label for screen reader users.
The external link opens in a new tab but doesn't announce this to screen readers. Adding an aria-label would improve accessibility.
♿ Optional accessibility enhancement
if (href) {
return (
- <a href={href} target="_blank" rel="noopener noreferrer" style={containerStyle}>
+ <a
+ href={href}
+ target="_blank"
+ rel="noopener noreferrer"
+ aria-label={`${message} (opens in new tab)`}
+ style={containerStyle}
+ >
{message} <span style={{ marginLeft: 4 }}>↗</span>
</a>
);
}📝 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.
Suggested change
| if (href) { | |
| return ( | |
| <a href={href} target="_blank" rel="noopener noreferrer" style={containerStyle}> | |
| {message} <span style={{ marginLeft: 4 }}>↗</span> | |
| </a> | |
| ); | |
| } | |
| if (href) { | |
| return ( | |
| <a | |
| href={href} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| aria-label={`${message} (opens in new tab)`} | |
| style={containerStyle} | |
| > | |
| {message} <span style={{ marginLeft: 4 }}>↗</span> | |
| </a> | |
| ); | |
| } |
🤖 Prompt for 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.
In `@00-LEGACY_EXAMPLES/session-keys/app/src/components/Alert.tsx` around lines 58
- 64, The anchor returned when href is present lacks an aria-label to announce
that the link opens in a new tab; update the JSX in the Alert component (the
branch that returns <a href={href} ... style={containerStyle}>) to include a
descriptive aria-label that mentions the link target and that it opens in a new
tab (e.g., use the message or a default text plus "opens in a new tab") so
screen readers are informed; ensure you still keep target="_blank" and
rel="noopener noreferrer".
Comment on lines
+539
to
+552
| setSessionExpiresAt(validUntilBN.toNumber()); | ||
| const totalMs = Math.round(performance.now() - txStart); | ||
| setTransactionSuccess({ | ||
| message: `[Base] Session created in ${totalMs}ms`, | ||
| explorerUrl: baseExplorerUrl(signature), | ||
| }); | ||
| } else { | ||
| setSessionTokenExists(true); | ||
| setTransactionSuccess(`Session created successfully`); | ||
| setSessionExpiresAt(validUntilBN.toNumber()); | ||
| const totalMs = Math.round(performance.now() - txStart); | ||
| setTransactionSuccess({ | ||
| message: `[Base] Session created in ${totalMs}ms`, | ||
| explorerUrl: baseExplorerUrl(signature), | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Refactor to eliminate duplicated success-message block.
The success-message logic is duplicated in both branches of the if (sessionTokenPDA.current) conditional. Extract it to run once after the conditional completes.
♻️ Consolidate success handling
}
setSessionTokenExists(true);
setSessionExpiresAt(validUntilBN.toNumber());
- const totalMs = Math.round(performance.now() - txStart);
- setTransactionSuccess({
- message: `[Base] Session created in ${totalMs}ms`,
- explorerUrl: baseExplorerUrl(signature),
- });
} else {
setSessionTokenExists(true);
setSessionExpiresAt(validUntilBN.toNumber());
- const totalMs = Math.round(performance.now() - txStart);
- setTransactionSuccess({
- message: `[Base] Session created in ${totalMs}ms`,
- explorerUrl: baseExplorerUrl(signature),
- });
}
+ const totalMs = Math.round(performance.now() - txStart);
+ setTransactionSuccess({
+ message: `[Base] Session created in ${totalMs}ms`,
+ explorerUrl: baseExplorerUrl(signature),
+ });🤖 Prompt for 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.
In `@session-keys/app/src/App.tsx` around lines 539 - 552, The two branches of the
if (sessionTokenPDA.current) duplicate the success handling; keep only the
branch-specific action setSessionTokenExists(true) in the else, then move the
shared logic (setSessionExpiresAt(validUntilBN.toNumber()), compute totalMs from
performance.now() - txStart, and setTransactionSuccess({ message: `[Base]
Session created in ${totalMs}ms`, explorerUrl: baseExplorerUrl(signature) })) to
execute once after the conditional. Update references to validUntilBN, txStart,
signature, baseExplorerUrl, setSessionExpiresAt, and setTransactionSuccess
accordingly so behavior is unchanged.
jonasXchen
changed the title
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 by CodeRabbit
New Features
Documentation
Chores