Automatically switch wallet network #275
Conversation
WalkthroughThe changes in this pull request involve modifications to the integration of MetaMask within the Monomer wallet application. The HTML structure of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebPage
participant MetaMask
User->>WebPage: Load page
WebPage->>MetaMask: Check for MetaMask
MetaMask-->>WebPage: MetaMask installed
WebPage->>MetaMask: Connect to MetaMask
MetaMask-->>WebPage: Connection successful
WebPage->>MetaMask: Switch to L1 Chain
MetaMask-->>WebPage: Chain switched
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
cmd/monogen/testapp/wallet/metamask-integration.js (2)
33-40: Consider more granular error handling.The connection flow looks good, but consider handling errors from
addL1ChainToMetaMaskandswitchToMonomerDevnetseparately to provide more specific error messages to users.async function connectMetamask() { try { await window.ethereum.request({ method: 'eth_requestAccounts' }); let provider = new ethers.providers.Web3Provider(window.ethereum); let signer = provider.getSigner(); accountSpan.innerText = await signer.getAddress(); optimismPortalContract = new ethers.Contract( optimismPortalAddress, optimismPortalAbi, signer ); - await addL1ChainToMetaMask(); - await switchToMonomerDevnet(); + try { + await addL1ChainToMetaMask(); + } catch (error) { + console.error("Error adding L1 chain:", error); + throw new Error("Failed to add L1 chain to MetaMask"); + } + + try { + await switchToMonomerDevnet(); + } catch (error) { + console.error("Error switching to Monomer Devnet:", error); + throw new Error("Failed to switch to Monomer Devnet"); + } console.log("Connected to MetaMask and contract instantiated."); } catch (error) { console.error("Error connecting to MetaMask:", error); } }
Line range hint
143-145: Consider user consent before auto-connecting.While automatic connection improves UX, it's generally recommended to wait for explicit user interaction before connecting to their wallet. This approach:
- Follows web3 best practices
- Respects user privacy
- Prevents unexpected wallet popups
Consider implementing a connection button that's automatically clicked only if the user has previously connected (using localStorage to remember their preference).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- cmd/monogen/testapp/wallet/index.html (0 hunks)
- cmd/monogen/testapp/wallet/metamask-integration.js (2 hunks)
💤 Files with no reviewable changes (1)
- cmd/monogen/testapp/wallet/index.html
🔇 Additional comments (2)
cmd/monogen/testapp/wallet/metamask-integration.js (2)
62-62: Consider security implications of hardcoded RPC URL.The chainId update looks good, but the hardcoded localhost RPC URL might cause issues in production environments.
Let's check for other RPC URL references:
#!/bin/bash # Search for RPC URL patterns rg "rpcUrls|127\.0\.0\.1:9001|localhost:9001" --type js
12-14: LGTM! Verify chain ID for Monomer Devnet.The initialization looks good. The chain ID is properly formatted as a hex string.
Let's verify if this chain ID is consistently used across the codebase:
✅ Verification successful
Chain ID
0x384is correctly used for Monomer DevnetThe verification confirms that:
- The chain ID
0x384is consistently used inmetamask-integration.js- It's properly identified as 'Ethereum (Monomer Devnet)' in the network configuration
- This is separate from the L2 chain configuration in Keplr integration which uses a different chain ID as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other chain ID references rg -i "0x384|900" --type jsLength of output: 160
Script:
#!/bin/bash # Let's search for chain configurations or network definitions rg -i "chainid|network.*id|chain.*id" --type js -A 3Length of output: 2419
| async function switchToMonomerDevnet() { | ||
| try { | ||
| await window.ethereum.request({ | ||
| method: "wallet_switchEthereumChain", | ||
| params: [ | ||
| { | ||
| chainId: chainId, | ||
| }, | ||
| ], | ||
| }); | ||
| } catch (error) { | ||
| console.error("Error switching Ethereum chain: ", error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for chain switching.
The function should handle specific error codes, particularly 4902 (unrecognized chain), and provide user feedback through the status paragraph.
async function switchToMonomerDevnet() {
try {
await window.ethereum.request({
method: "wallet_switchEthereumChain",
params: [{ chainId: chainId }],
});
+ statusParagraph.innerText = 'Successfully switched to Monomer Devnet';
} catch (error) {
- console.error("Error switching Ethereum chain: ", error);
+ if (error.code === 4902) {
+ console.error("Unrecognized chain. Adding chain first...");
+ statusParagraph.innerText = 'Adding Monomer Devnet...';
+ await addL1ChainToMetaMask();
+ return switchToMonomerDevnet();
+ }
+ console.error("Error switching Ethereum chain:", error);
+ statusParagraph.innerText = `Failed to switch chain: ${error.message}`;
+ throw error;
}
}Committable suggestion was skipped due to low confidence.
joshklop
force-pushed
the
joshklop.auto-switch-network
branch
from
8a70bf0 to
fd1ac47
Compare
joshklop
force-pushed
the
joshklop.auto-switch-network
branch
from
fd1ac47 to
8923c99
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
cmd/monogen/testapp/wallet/index.html (2)
114-114: Consider clarifying the page reload instruction.While adding the reload instruction is helpful, it would be more informative to explain why reloading is necessary (e.g., "Finally, reload the page to ensure proper wallet connection").
- chain. Finally, reload the page. + chain. Finally, reload the page to ensure proper wallet connection and state reset.
Line range hint
89-116: Add clarity about automatic network switching.Since the MetaMask integration now handles network switching automatically (as indicated by the removal of the manual button), consider adding a note about this behavior in the instructions to set proper user expectations.
Add this line after the existing instructions:
chain. Finally, reload the page. + </li> + <li> + Note: The network will be configured automatically when you interact with the wallet. No manual network addition is required.cmd/monogen/testapp/wallet/metamask-integration.js (2)
14-14: Remove unnecessary undefined initialization.In JavaScript, variables declared without initialization are automatically undefined.
-let optimismPortalContract = undefined; +let optimismPortalContract;
Based on the results, I can now generate the final response:
Consider abstracting wallet-specific implementation for better L1 wallet compatibility
While the code uses the standard EIP-1193
window.ethereuminterface, the implementation is tightly coupled to MetaMask with specific function names and error messages. Consider:
cmd/monogen/testapp/wallet/metamask-integration.js:
- Functions like
connectMetamask()andaddL1ChainToMetaMask()are MetaMask-specific- Error messages explicitly mention MetaMask: "Please install Metamask"
- Direct calls to MetaMask-specific RPC methods like
wallet_switchEthereumChainandwallet_addEthereumChainTo improve compatibility, consider:
- Creating a wallet interface abstraction
- Using provider-agnostic function names and messages
- Moving MetaMask-specific logic into a separate implementation
🔗 Analysis chain
Line range hint
1-1: Verify compatibility with other L1 wallets.The implementation follows EIP-1193 standard and should work with other L1 wallets. Let's verify the presence of alternative wallet providers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for references to specific wallet providers # Expected: Only standard EIP-1193 interfaces are used echo "Checking for wallet-specific implementations..." rg -i "metamask|coinbase|wallet" --type js echo "Verifying EIP-1193 standard usage..." rg "window\.ethereum" --type jsLength of output: 2155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
cmd/monogen/testapp.zipis excluded by!**/*.zip
📒 Files selected for processing (2)
- cmd/monogen/testapp/wallet/index.html (1 hunks)
- cmd/monogen/testapp/wallet/metamask-integration.js (2 hunks)
🔇 Additional comments (2)
cmd/monogen/testapp/wallet/metamask-integration.js (2)
12-12: Add decimal value comment for chainId.For better readability, add a comment with the decimal representation of the chain ID.
-const chainId = "0x384"; // 900 +const chainId = "0x384"; // 900 (decimal)
42-54: Previous review comment about enhanced error handling for chain switching is still valid.
| await addL1ChainToMetaMask(); | ||
| await switchToMonomerDevnet(); | ||
|
|
||
| console.log("Connected to MetaMask and contract instantiated."); | ||
| } catch (error) { | ||
| console.error("Error connecting to MetaMask:", error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance error handling in connectMetamask.
The error handling should provide user feedback through the status paragraph and handle specific error codes.
await addL1ChainToMetaMask();
await switchToMonomerDevnet();
console.log("Connected to MetaMask and contract instantiated.");
+statusParagraph.innerText = "Successfully connected to MetaMask on Monomer Devnet";
} catch (error) {
- console.error("Error connecting to MetaMask:", error);
+ console.error("Error connecting to MetaMask:", error);
+ if (error.code === 4001) {
+ statusParagraph.innerText = "Connection rejected by user";
+ } else {
+ statusParagraph.innerText = `Failed to connect: ${error.message}`;
+ }
+ throw error;
}Committable suggestion was skipped due to low confidence.
| } | ||
|
|
||
| async function addL1ChainToMetaMask() { | ||
| try { | ||
| await window.ethereum.request({ | ||
| method: 'wallet_addEthereumChain', | ||
| params: [{ | ||
| chainId: '0x384', // 900 | ||
| chainId: chainId, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making RPC URL configurable.
The RPC URL is currently hardcoded to 'http://127.0.0.1:9001'. Consider making it configurable to support different environments (development, staging, production).
+const MONOMER_RPC_URL = process.env.MONOMER_RPC_URL || 'http://127.0.0.1:9001';
async function addL1ChainToMetaMask() {
try {
await window.ethereum.request({
method: 'wallet_addEthereumChain',
params: [{
chainId: chainId,
chainName: 'Ethereum (Monomer Devnet)',
nativeCurrency: {
name: 'ETH',
symbol: 'ETH',
decimals: 18
},
- rpcUrls: ['http://127.0.0.1:9001'],
+ rpcUrls: [MONOMER_RPC_URL],
blockExplorerUrls: []
}]
});📝 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.
| chainId: chainId, | |
| chainId: chainId, | |
| chainName: 'Ethereum (Monomer Devnet)', | |
| nativeCurrency: { | |
| name: 'ETH', | |
| symbol: 'ETH', | |
| decimals: 18 | |
| }, | |
| rpcUrls: [MONOMER_RPC_URL], | |
| blockExplorerUrls: [] |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
chainIdto improve code maintainability and consistency.