feat: make phantom great again - #771
Conversation
Phantom extension rate limits when requestAccounts() and other provider methods are called too frequently during account discovery. This fix adds caching for BTC, ETH, and Solana addresses to prevent repeated requests. Changes: - Cache Bitcoin address after first requestAccounts() call - Cache Solana address after first connect() call - Reuse existing ETH address caching mechanism This allows all three chains (BTC, EVM, Solana) to work simultaneously without triggering Phantom's rate limiting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add PhantomSuiProvider type interface - Implement suiGetAddress and suiSignTx methods - Add Sui provider detection in adapter - Enable caching for Sui addresses to prevent rate limiting - Follow same pattern as Solana implementation
- Enable _supportsEthSwitchChain flag - Implement ethGetChainId to get current chain - Implement ethSwitchChain for supported chains (Ethereum, Base, Polygon) - Add request method to PhantomEvmProvider type - Only allow switching between Phantom's supported chains - Properly handle errors when chain is not supported
|
Warning Rate limit exceeded@gomesalexandre has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (27)
📝 WalkthroughWalkthroughThis PR adds Phantom wallet support for the Sui blockchain. It extends the core SuiSignTx interface with a transactionJson field, implements SuiWallet and SuiWalletInfo interfaces in the Phantom adapter, integrates PhantomSuiProvider, adds account path and address management methods, and defines Sui signing operations including transaction and message signing with signature validation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PhantomHDWallet
participant PhantomSuiProvider
participant SuiNetwork
rect rgb(200, 220, 240)
Note over Client,SuiNetwork: suiGetAddress Flow
Client->>PhantomHDWallet: suiGetAddress()
alt Address Cached
PhantomHDWallet->>PhantomHDWallet: Return cached suiAddress
else Address Not Cached
PhantomHDWallet->>PhantomSuiProvider: requestAccount()
PhantomSuiProvider->>SuiNetwork: Request Account
SuiNetwork-->>PhantomSuiProvider: { address, publicKey }
PhantomSuiProvider-->>PhantomHDWallet: Account Data
PhantomHDWallet->>PhantomHDWallet: Cache address
PhantomHDWallet-->>Client: Return address | null
end
end
rect rgb(220, 240, 200)
Note over Client,SuiNetwork: suiSignTx Flow
Client->>PhantomHDWallet: suiSignTx(transactionJson, ...)
PhantomHDWallet->>PhantomSuiProvider: requestAccount()
PhantomSuiProvider->>SuiNetwork: Request Account
SuiNetwork-->>PhantomSuiProvider: { address, publicKey }
PhantomHDWallet->>PhantomSuiProvider: signTransaction({ transactionJson, address, networkID })
PhantomSuiProvider->>SuiNetwork: Sign Transaction
SuiNetwork-->>PhantomSuiProvider: 97-byte signature payload
PhantomSuiProvider->>PhantomSuiProvider: Validate & Extract (signature + publicKey)
PhantomSuiProvider-->>PhantomHDWallet: Hex-encoded { signature, publicKey }
PhantomHDWallet-->>Client: SuiSignedTx | null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
gomesalexandre
changed the title
…ch native wallet format
- Remove debug console.log statements - Remove AI-generated comments for consistency with other files - Use early throw pattern for better readability - Remove unnecessary 'as string' cast - Improve types in types.ts (transaction is string, not any) - Make transactionJson non-optional in core types for consistency
- Remove unnecessary AI-generated comments throughout - Simplify if-else blocks with early returns - Remove redundant transactionJson conditional since it's now required - Add clarifying comment for 97-byte signature format - Improve code consistency with rest of codebase 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove non-existent signPersonalMessage method - Remove unnecessary providers.ExternalProvider inheritance - Fix suiSignMessage to use existing signMessage method 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove unnecessary curly braces for single-line returns - Use unknown instead of any for better type safety in request method - Remove unused PhantomSuiProvider methods (isPhantom, signAndExecuteTransaction) - Keep only the methods we actually use to maintain lean type definitions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…/master Reverting version bumps and package changes back to master state to ensure clean package management state. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/hdwallet-core/src/sui.ts (1)
9-15: MaketransactionJsonoptional in theSuiSignTxinterface.The
transactionJsonfield is required but only used by the Phantom wallet implementation. The native wallet (hdwallet-native) and Ledger wallet (hdwallet-ledger) implementations both ignore this field and only useintentMessageBytes. ChangetransactionJson: string;totransactionJson?: string;to avoid requiring callers to provide a field that their chosen wallet implementation may not need.
🧹 Nitpick comments (3)
packages/hdwallet-phantom/src/sui.ts (2)
13-39: Consider error handling for provider calls.The function doesn't handle potential errors from
provider.requestAccount()orprovider.signTransaction(). If the user rejects the request or the provider fails, an unhandled exception will propagate. Consider wrapping in try-catch to returnnullconsistently with error logging.export async function suiSignTx(msg: core.SuiSignTx, provider: PhantomSuiProvider): Promise<core.SuiSignedTx | null> { - const account = await provider.requestAccount(); - - const result = await provider.signTransaction({ - transaction: msg.transactionJson, - address: account.address, - networkID: "sui:mainnet", - }); + try { + const account = await provider.requestAccount(); + + const result = await provider.signTransaction({ + transaction: msg.transactionJson, + address: account.address, + networkID: "sui:mainnet", + }); + + const fullSignatureBuffer = Buffer.from(result.signature, "base64"); + // ... rest of implementation + } catch (error) { + console.error("Failed to sign Sui transaction:", error); + return null; + }
16-20: Hardcoded mainnet network ID.The
networkIDis hardcoded to"sui:mainnet". This works for production but may limit testnet/devnet usage during development. Consider making this configurable if testnet support is needed in the future.packages/hdwallet-phantom/src/phantom.ts (1)
358-366: Add type validation for chain ID response.
evmProvider.requestreturnsPromise<unknown>. The code assumes the response is a hex string but doesn't validate. Consider adding type checking to avoid runtime errors with malformed responses.public async ethGetChainId(): Promise<number | null> { try { - const chainIdHex = await this.evmProvider.request({ method: "eth_chainId" }); - return parseInt(chainIdHex, 16); + const chainIdHex = await this.evmProvider.request({ method: "eth_chainId" }); + if (typeof chainIdHex !== "string") return null; + return parseInt(chainIdHex, 16); } catch (error) { console.error("Failed to get chain ID from Phantom:", error); return null; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
packages/hdwallet-core/src/sui.ts(1 hunks)packages/hdwallet-native/src/__tests__/sui.test.ts(1 hunks)packages/hdwallet-phantom/src/adapter.ts(3 hunks)packages/hdwallet-phantom/src/index.ts(1 hunks)packages/hdwallet-phantom/src/phantom.ts(7 hunks)packages/hdwallet-phantom/src/sui.ts(1 hunks)packages/hdwallet-phantom/src/types.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-20T11:04:44.808Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 737
File: packages/hdwallet-trezor/src/ethereum.ts:122-138
Timestamp: 2025-11-20T11:04:44.808Z
Learning: In packages/hdwallet-trezor/src/ethereum.ts, the ethSignTypedData function correctly returns the signature from res.payload.signature without adding a "0x" prefix. This works correctly in practice and has been tested, despite appearing inconsistent with ethSignMessage which does add the prefix. The Trezor Connect ethereumSignTypedData response already provides the signature in the correct format for consumption.
Applied to files:
packages/hdwallet-core/src/sui.tspackages/hdwallet-native/src/__tests__/sui.test.ts
📚 Learning: 2025-10-15T23:22:26.842Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Applied to files:
packages/hdwallet-core/src/sui.ts
📚 Learning: 2025-12-12T11:19:53.263Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 769
File: packages/hdwallet-walletconnectV2/src/walletconnectV2.ts:150-152
Timestamp: 2025-12-12T11:19:53.263Z
Learning: In the shapeshift/hdwallet monorepo, remove reliance on ethSupportsNetwork() across wallet implementations. This legacy method is no longer used to determine chain support. Instead, rely on the wallet class flags like _supportsMonad, _supportsPlasma, _supportsHyperEvm. Review all wallet implementations for ethSupportsNetwork() usage and migrate checks to the corresponding _supports* flags, updating tests and any affected logic accordingly.
Applied to files:
packages/hdwallet-core/src/sui.tspackages/hdwallet-phantom/src/index.tspackages/hdwallet-phantom/src/sui.tspackages/hdwallet-native/src/__tests__/sui.test.tspackages/hdwallet-phantom/src/types.tspackages/hdwallet-phantom/src/adapter.tspackages/hdwallet-phantom/src/phantom.ts
📚 Learning: 2025-08-07T15:47:29.207Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger/src/transport.ts:10-10
Timestamp: 2025-08-07T15:47:29.207Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the code works locally without TypeScript errors but has issues in CI environment. Using ts-expect-error would fail locally since there are no actual errors to suppress.
Applied to files:
packages/hdwallet-native/src/__tests__/sui.test.tspackages/hdwallet-phantom/src/adapter.ts
📚 Learning: 2025-08-07T15:47:26.835Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger-webusb/src/transport.ts:12-12
Timestamp: 2025-08-07T15:47:26.835Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the CI environment has different type checking behavior than local development. The code works locally without errors, but CI reports type issues, so ts-ignore is necessary to suppress the inconsistent type checking across environments.
Applied to files:
packages/hdwallet-native/src/__tests__/sui.test.ts
📚 Learning: 2025-12-12T11:19:46.121Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 769
File: packages/hdwallet-ledger/src/ledger.ts:403-405
Timestamp: 2025-12-12T11:19:46.121Z
Learning: In packages/hdwallet-ledger/src/ethereum.ts, the ethSupportsNetwork function is a legacy/unused function that only returns true for chainId === 1. The Ledger ETH module does not call ethSupportsNetwork to validate chain support during signing operations - it accepts any chainId passed in the ETHSignTx message directly, so chain support flags can be enabled without needing to update ethSupportsNetwork.
Applied to files:
packages/hdwallet-phantom/src/phantom.ts
🧬 Code graph analysis (2)
packages/hdwallet-phantom/src/sui.ts (3)
packages/hdwallet-phantom/src/phantom.ts (2)
suiGetAddress(463-472)suiSignTx(474-477)packages/hdwallet-phantom/src/types.ts (1)
PhantomSuiProvider(35-39)packages/hdwallet-core/src/sui.ts (2)
SuiSignTx(9-15)SuiSignedTx(17-20)
packages/hdwallet-phantom/src/adapter.ts (2)
packages/hdwallet-phantom/src/types.ts (4)
PhantomEvmProvider(7-12)PhantomUtxoProvider(14-26)PhantomSolanaProvider(28-33)PhantomSuiProvider(35-39)packages/hdwallet-phantom/src/phantom.ts (1)
PhantomHDWallet(187-478)
🔇 Additional comments (8)
packages/hdwallet-native/src/__tests__/sui.test.ts (1)
49-53: LGTM!The test correctly adds the new
transactionJsonfield required by the updatedSuiSignTxinterface.packages/hdwallet-phantom/src/index.ts (1)
1-3: LGTM!Barrel export correctly exposes the new Sui module.
packages/hdwallet-phantom/src/types.ts (1)
35-39: LGTM!The
PhantomSuiProvidertype correctly models Phantom's Sui API with appropriate method signatures for account retrieval, message signing, and transaction signing.packages/hdwallet-phantom/src/adapter.ts (1)
32-48: LGTM!The adapter correctly treats
suiProvideras optional - it's not included in the required providers check (line 38), matching the optional declaration in the global interface and the optional constructor parameter inPhantomHDWallet.packages/hdwallet-phantom/src/phantom.ts (4)
174-184: LGTM!Sui account path methods correctly delegate to core utilities, following the same pattern as Solana.
191-207: LGTM!Chain support flags correctly enable Polygon, Monad, HyperEVM, and Sui. The
_supportsEthSwitchChainflag enables the chain switching feature. Based on learnings, using these_supports*flags is the correct modern approach rather thanethSupportsNetwork().
440-448: LGTM!Address caching for Solana correctly uses
!== undefinedto distinguish between "not yet fetched" and "fetched but failed", preventing repeated provider calls that cause rate limiting.
462-477: LGTM!Sui wallet methods correctly:
- Guard against missing
suiProvider- Implement address caching to prevent rate limiting
- Delegate signing to the dedicated
suimodule
NeOMakinG
left a comment
NeOMakinG
left a comment
There was a problem hiding this comment.
My record failed because my computed ran out of battery but, everything works fine (did a bunch of swaps on multiple chains)
The only weird thing I noticed is my HYPE balance isn't showing up in the swapper:
while I can see it in the asset list:
I think we are missing something for the hype support probably on web side, that is going to be better after the last PR adding the notion of second class chain in a better way, happy to stamp it and test in the last PR!
@coderabbitai @shapeshift/operations have been mentioning this one - haven't been able to repro this one when devving HyperEVM things today and testing but may be depending on wallet support (e.g could happen with Phantom only, or maybe not happening anymore following latest PRs). Either way, open an issue in shapeshift/web to double check this in more deets and fix if still happening, or close. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@gomesalexandre, I'll create an issue in shapeshift/web to track this potential HyperEVM issue that operations has been mentioning. Since you haven't been able to reproduce it, the issue will document the need for further investigation to determine if it's still occurring or was resolved by recent PRs. 🏁 Script executed: Length of output: 46 @gomesalexandre, I've created issue #11430 in shapeshift/web to track the investigation of the potential HyperEVM issue reported by operations. The issue includes references to this PR and your comment, and is assigned to you. The team can now verify if the issue still exists or has been resolved by recent changes. 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@gomesalexandre I'll create an issue in shapeshift/web to track this HyperEVM support concern for verification. 🏁 Script executed: Length of output: 46 @gomesalexandre I've created issue #11431 in shapeshift/web to track the HyperEVM support verification. The issue includes:
This will help ensure the issue gets properly verified and either fixed or closed based on testing results. 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@gomesalexandre I'll create an issue in shapeshift/web to track this HyperEVM support concern for further investigation. 🏁 Script executed: Length of output: 46 @gomesalexandre I've created issue #11432 in shapeshift/web to track the investigation of the HyperEVM support concern mentioned by operations. The issue includes context from this PR and a checklist to verify whether the issue still exists or has been resolved by recent changes. 🧠 Learnings used |
2 participants
Summary
Web fren of shapeshift/web#11416, test me with it
This PR:
Changes
Test plan
Issue
Screenshots
develop
this diff
https://jam.dev/c/f5ffdde3-da4f-4848-9b8c-296ec7a0ecc2
https://jam.dev/c/3829ae7a-98b8-44be-a359-2344fa2fc6a6
https://jam.dev/c/e2136755-29b1-4842-9cff-b1f043458a9a
https://jam.dev/c/0d6d38d2-ec17-41e7-ae8e-6d726577e5be
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.