Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughRefactors RN authentication from private-key retrieval to EOA-based custom accounts and signing via WebView. Adds signing helpers, BigInt-safe serialization, EIP‑7702 authorization support, wires Changes
Sequence DiagramsequenceDiagram
participant App as Main
participant Auth as Authorized
participant Hook as useWalletModeVerification
participant Kit as Transaction Kit
participant Sign as pillarWalletMessaging
participant WebView as RN WebView
App->>App: read eoaAddress (URL / localStorage)
App->>App: toAccount(...) -> customAccount
App->>Auth: render with eoaAddress, customAccount
Auth->>Kit: set viemLocalAccount = customAccount
Auth->>Hook: verify wallet mode (eoaAddress)
Hook->>Hook: resolveEoaAddress (prefer eoaAddress)
Hook->>Kit: on-chain checks with resolvedEoaAddress
Note over App,WebView: signing request flow
App->>Sign: signMessageViaWebView(payload)
Sign->>WebView: postMessage({type: pillarXSigningRequest,...})
WebView->>Sign: postMessage({type: pillarWalletSigningResponse, value:{result}})
Sign->>App: resolve signature (parse BigInt, SignedAuthorization if needed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Deploying x with
|
| Latest commit: |
cae5279
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://861e64a3.x-e62.pages.dev |
| Branch Preview URL: | https://feature-7702-remote-signing.x-e62.pages.dev |
github-actions
bot
temporarily deployed
to
Preview (feature/7702-remote-signing)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.json(1 hunks)src/components/BottomMenu/index.tsx(2 hunks)src/components/BottomMenuModal/AccountModal.tsx(3 hunks)src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(1 hunks)src/containers/Authorized.tsx(4 hunks)src/containers/Main.tsx(10 hunks)src/hooks/useWalletModeVerification.tsx(5 hunks)src/pages/Landing.jsx(2 hunks)src/utils/__tests__/pillarWalletMessaging.test.ts(1 hunks)src/utils/pillarWalletMessaging.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/components/BottomMenu/index.tsxsrc/hooks/useWalletModeVerification.tsxsrc/containers/Authorized.tsxsrc/containers/Main.tsx
📚 Learning: 2025-10-15T10:31:06.760Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 432
File: src/containers/Main.tsx:135-141
Timestamp: 2025-10-15T10:31:06.760Z
Learning: In src/containers/Main.tsx, the React Native webview messaging setup intentionally only activates when the `devicePlatform` URL parameter is present ('ios' or 'android'). On reloads or direct navigation without URL params, the site should behave normally and not attempt to re-establish RN messaging. This is by design to ensure the site functions properly in both RN webview and standard browser contexts.
Applied to files:
src/pages/Landing.jsxsrc/containers/Main.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/containers/Authorized.tsxsrc/containers/Main.tsx
🧬 Code graph analysis (2)
src/utils/__tests__/pillarWalletMessaging.test.ts (1)
src/utils/pillarWalletMessaging.ts (4)
requestSigning(88-154)signMessageViaWebView(161-179)signTransactionViaWebView(186-190)signAuthorizationViaWebView(206-256)
src/containers/Main.tsx (4)
src/utils/pillarWalletMessaging.ts (4)
signMessageViaWebView(161-179)signTransactionViaWebView(186-190)signTypedDataViaWebView(197-199)signAuthorizationViaWebView(206-256)src/utils/eip7702Authorization.ts (1)
OUR_EIP7702_IMPLEMENTATION_ADDRESS(5-6)src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(71-94)src/utils/blockchain.ts (1)
visibleChains(161-163)
src/containers/Main.tsx
Outdated
| const { isConnected: wagmiIsConnected } = useAccount(); | ||
| const [provider, setProvider] = useState<WalletClient | undefined>(undefined); | ||
| const [chainId, setChainId] = useState<number | undefined>(undefined); | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars |
There was a problem hiding this comment.
Do we want to keep privateKey and pkAccount? Because those were removed from other parts of the code in this PR (BottomMenuModal)
There was a problem hiding this comment.
Whoops i forgot to remove the PK stuff here, i've removed it now
|
|
||
| const walletChainId = 1; // default chain id is 1 (mainnet) | ||
|
|
||
| const newProvider = createWalletClient({ |
There was a problem hiding this comment.
Is it robust enough to set the provider on http() only (so public rpc?)
There was a problem hiding this comment.
Yes i would say so as this is the default that is shipped with all Viem installations. I don't actually think, in the end, that this http() is even used as ultimately it's the bundler that will make the transactions and anything else will do read operations using public RPC endpoints which is fine.
| }); | ||
|
|
||
| return; | ||
| } // END if (eoaAddress) |
There was a problem hiding this comment.
That's just something i add in my code to know which block i'm dealing with when i reach the end here - it's just for readability
| const searchParams = new URLSearchParams(window.location.search); | ||
| const devicePlatformFromUrl = searchParams.get('devicePlatform'); | ||
| const devicePlatformFromStorage = localStorage.getItem('DEVICE_PLATFORM'); | ||
| const eoaAddressFromUrl = searchParams.get('eoaAddress'); |
There was a problem hiding this comment.
Just checking on this, if a user is connected on pillarX with Privy, and then decides (edge case) to put their EOA address or ANY address in the url, will that not clash at some point?
There was a problem hiding this comment.
the updateProvider has a priorty order - and in this case PillarX will load with the EOA address as it's priority. They just wouldn't be able to make any transactions
github-actions
bot
requested a deployment
to
Preview (feature/7702-remote-signing)
github-actions
bot
requested a deployment
to
Preview (feature/7702-remote-signing)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/containers/Main.tsx (2)
249-286: Review type assertions in signing methods.Lines 276 and 285 use type assertions that could hide type mismatches:
- Line 276:
transaction as TransactionSerializable- Line 285:
typedData as unknown as TypedData(double assertion)Consider validating types or using type guards:
async signTransaction(transaction) { - return signTransactionViaWebView( - transaction as TransactionSerializable - ); + // Validate transaction shape before passing to WebView + if (!transaction || typeof transaction !== 'object') { + throw new Error('Invalid transaction object'); + } + return signTransactionViaWebView(transaction as TransactionSerializable); }, async signTypedData(typedData) { - return signTypedDataViaWebView(typedData as unknown as TypedData); + // Validate typedData shape + if (!typedData || typeof typedData !== 'object') { + throw new Error('Invalid typed data'); + } + return signTypedDataViaWebView(typedData as TypedData); },
322-346: Consider removing or documenting the commented verification code.Lines 322-346 contain extensive commented-out signature verification logic. While the inline comment explains it's kept for potential future use, commented code can create maintenance burden and confusion.
Consider one of these approaches:
- Remove the commented code and rely on git history if needed later
- If verification is genuinely useful for debugging, extract it to a separate utility function that can be conditionally enabled via a feature flag or environment variable
- Add a more prominent comment explaining under what circumstances this verification should be uncommented and used
- /** - * NOTE: This is commented out but is being left in - * just incase we need to verify the signature again - * in future. - */ - // Optional: Verify the signature (for debugging) - // Serialize the signature to hex format for verification - // if (!signedAuthorization.v) { - // throw new Error('Signature missing v value'); - // } - // const vHex = signedAuthorization.v.toString(16).padStart(2, '0'); - // ... + // Signature verification was removed - see git history (commit XYZ) if needed for debugging
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/containers/Main.tsx(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/containers/Main.tsx
📚 Learning: 2025-10-15T10:31:06.760Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 432
File: src/containers/Main.tsx:135-141
Timestamp: 2025-10-15T10:31:06.760Z
Learning: In src/containers/Main.tsx, the React Native webview messaging setup intentionally only activates when the `devicePlatform` URL parameter is present ('ios' or 'android'). On reloads or direct navigation without URL params, the site should behave normally and not attempt to re-establish RN messaging. This is by design to ensure the site functions properly in both RN webview and standard browser contexts.
Applied to files:
src/containers/Main.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/containers/Main.tsx
🧬 Code graph analysis (1)
src/containers/Main.tsx (2)
src/utils/pillarWalletMessaging.ts (4)
signMessageViaWebView(161-179)signTransactionViaWebView(186-190)signTypedDataViaWebView(197-199)signAuthorizationViaWebView(206-256)src/utils/eip7702Authorization.ts (1)
OUR_EIP7702_IMPLEMENTATION_ADDRESS(5-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: unit-tests
🔇 Additional comments (6)
src/containers/Main.tsx (6)
12-20: LGTM: Imports align with remote signing architecture.The new imports support the custom account flow with WebView signing delegation and EIP-7702 authorization. All imports are utilized in the custom account setup.
Also applies to: 36-41, 60-60
85-88: LGTM: State variables support the new authentication model.The
eoaAddressandcustomAccountstates correctly replace the previous private-key-based approach, aligning with the PR's objective of remote signing.
127-204: LGTM: WebView messaging setup correctly extracts and persistseoaAddress.The logic properly handles URL parameters, localStorage persistence, and React Native context detection. The removal of private key requests aligns with PR objectives. (Note: Address validation should be added as flagged in line 92 review.)
730-730: LGTM: Dependency array correctly includeseoaAddress.The addition of
eoaAddressto the dependency array ensures the provider is re-initialized when the EOA address changes, which is correct given thateoaAddress-based authentication takes priority over other methods.
765-766: LGTM: New props correctly wired toAuthorizedcomponent.The
eoaAddressandcustomAccountprops enable the Authorized component to leverage the custom account for transaction kit configuration and wallet mode verification.
292-320: No changes needed. Authorization parameter handling is correct.The fallback to
OUR_EIP7702_IMPLEMENTATION_ADDRESS('0xd6CEDDe84be40893d153Be9d467CD6aD37875b28') is the correct Kernel V3.3 account implementation address for EIP-7702 delegation. The conditional parameter logic correctly ensures only one ofcontractAddressoraddressis passed to viem'sAuthorizationRequest, preventing invalid state. The implementation is consistent with its usage inuseWalletModeVerification.tsxwhere the same constant validates delegation addresses. While explicit address validation in the JavaScript layer isn't present, viem's type system and the React Native signing app provide implicit validation.
| const previouslyAuthenticated = !!localStorage.getItem('privy:token'); | ||
| const isAppReady = ready && !isLoadingAllowedApps; | ||
| const isAuthenticated = authenticated || wagmiIsConnected || !!pkAccount; | ||
| const isAuthenticated = authenticated || wagmiIsConnected || !!eoaAddress; |
There was a problem hiding this comment.
Validate eoaAddress format before considering user authenticated.
The eoaAddress (extracted from URL parameters on line 132) is treated as sufficient for authentication without format validation. A malformed address could cause issues in downstream operations.
Consider adding validation:
+import { isAddress } from 'viem';
+
const isAuthenticated = authenticated || wagmiIsConnected || !!eoaAddress;
+
+// Validate eoaAddress format if present
+useEffect(() => {
+ if (eoaAddress && !isAddress(eoaAddress)) {
+ console.error('Invalid EOA address format:', eoaAddress);
+ localStorage.removeItem('EOA_ADDRESS');
+ setEoaAddress(undefined);
+ }
+}, [eoaAddress]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/containers/Main.tsx around line 92 (and note eoaAddress is extracted
around line 132), the code currently treats any eoaAddress truthy value as proof
of authentication; validate the address format first and only count it as
authentication if valid. Update the logic so isAuthenticated uses a proper
address validator (e.g., ethers.utils.isAddress or a strict regex) against
eoaAddress, optionally normalizing/checksumming it before use; if validation
fails, ignore the parameter (do not set authenticated via eoaAddress) and
log/handle the invalid value where appropriate.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Improvements
Chores