Update for controller 0.5.0 (without starknet-react v3) #282
Conversation
- update controller interface
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces significant updates across multiple files in the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2
🧹 Outside diff range and nitpick comments (4)
ui/src/app/provider.tsx (1)
Line range hint
40-46: Consider adding error handling for missing network config parameters.The network configuration parameters are accessed with the non-null assertion operator (
!), which could lead to runtime errors if the network configuration is incomplete.Consider adding validation:
- controllerConnector( - networkConfig[network!].gameAddress, - networkConfig[network!].lordsAddress, - networkConfig[network!].ethAddress, - networkConfig[network!].rpcUrl - ), + (() => { + const config = networkConfig[network!]; + if (!config?.gameAddress || !config?.lordsAddress || + !config?.ethAddress || !config?.rpcUrl) { + throw new Error(`Incomplete network configuration for ${network}`); + } + return controllerConnector( + config.gameAddress, + config.lordsAddress, + config.ethAddress, + config.rpcUrl + ); + })(),ui/src/app/lib/connectors.ts (1)
Line range hint
58-98: Review and document the security implications of approved methods.The policies array defines a whitelist of contract methods that can be called. This is security-critical configuration that should be well-documented and carefully reviewed.
Consider:
- Adding comments explaining the security implications of each approved method
- Documenting the process for updating this whitelist
- Adding validation for the contract addresses passed to the function
ui/src/app/components/onboarding/Sections/WalletSection.tsx (1)
21-23: Inconsistent naming between code and UI textThe connector ID uses "controller" while the button text still displays "Cartridge Controller". This inconsistency might be confusing for maintenance. Consider updating the button text to match the new terminology.
- Login with Cartridge Controller + Login with ControllerAlso applies to: 61-64, 83-86
ui/src/app/page.tsx (1)
Line range hint
279-289: Consider improving the controller initialization logicWhile the implementation works, there are a few potential improvements:
- The connector ID check using
includes("controller")might be too permissive. Consider using a more specific check.- The
username()call lacks error handling.Consider this more robust implementation:
- if (connector?.id.includes("controller")) { + if (connector?.id === "controller") { // Use exact match setIsController(true); - init(); + try { + await init(); + } catch (error) { + console.error("Failed to initialize controller:", error); + setIsController(false); + setControllerDelegate(""); + setUsername(""); + } } else { setIsController(false); setControllerDelegate(""); setUsername(""); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
- ui/package.json (1 hunks)
- ui/src/app/components/onboarding/Sections/WalletSection.tsx (4 hunks)
- ui/src/app/components/profile/ProfileDialog.tsx (2 hunks)
- ui/src/app/lib/connectors.ts (2 hunks)
- ui/src/app/page.tsx (2 hunks)
- ui/src/app/provider.tsx (2 hunks)
🔇 Additional comments (11)
ui/src/app/provider.tsx (2)
Line range hint
40-46: LGTM: Connector configuration is properly updated.The controllerConnector is correctly initialized with all required network parameters.
2-9: Verify @cartridge/connector version compatibility.The import changes align with the controller update. Let's verify the dependency version compatibility.
✅ Verification successful
Import changes are correctly aligned with dependencies
The verification confirms that:
@cartridge/connectorversion0.5.0-alpha.0is present inui/package.json- The
controllerConnectorimport is correctly sourced from the local path@/app/lib/connectors- The implementation in
ui/src/app/lib/connectors.tsshows proper integration with@cartridge/connector/controllerThe import changes are consistent with the codebase and the dependency versions align with the controller 0.5.0 update mentioned in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json for @cartridge/connector version echo "Checking package.json for @cartridge/connector version:" cat package.json | grep -A 5 "@cartridge/connector"Length of output: 217
Script:
#!/bin/bash # Let's first find the package.json file echo "Looking for package.json files:" fd "package.json" --type f # Let's also check if the import path exists echo -e "\nChecking for controllerConnector import:" rg "controllerConnector" --type ts --type tsx -C 2Length of output: 291
Script:
#!/bin/bash # Check ui/package.json for dependencies echo "Checking ui/package.json for dependencies:" cat ui/package.json | jq '.dependencies' # Check for controllerConnector without file type restriction echo -e "\nChecking for controllerConnector usage:" rg "controllerConnector" -C 2 # Check for any connector-related imports echo -e "\nChecking for connector-related imports:" rg "import.*connector" -i -C 2Length of output: 9599
ui/package.json (2)
13-13: Verify alpha version usage in production.The update to
0.5.0-alpha.0introduces an alpha (pre-release) version. Alpha releases are typically unstable and not recommended for production use. Additionally, the exact version pinning (removing ^) prevents receiving bug fixes.Consider:
- Waiting for a stable release
- Or if alpha is required, document the known limitations and test thoroughly
- Using
^0.5.0-alpha.0to receive critical fixes within the alpha series
13-13: Verify compatibility with Starknet dependencies.The controller update might have compatibility requirements with Starknet-related packages. Let's verify the dependencies.
Also applies to: 15-16
ui/src/app/lib/connectors.ts (3)
51-57: Verify impact of removed paymaster configuration.The paymaster configuration has been removed in this update. This could affect how transaction fees are handled.
#!/bin/bash # Description: Look for paymaster-related code and documentation # Expected: Find evidence of how transaction fees are now handled # Search for paymaster references in the codebase echo "Checking for paymaster usage:" rg -l "paymaster|transaction.*fee" --type ts # Look for migration guides or documentation echo "Checking for migration documentation:" fd -e md . | xargs rg -l "0\.5\.0|migration|upgrade"
57-57:⚠️ Potential issueConsider removing unsafe type assertion.
The
as never as Connectortype assertion is a code smell that bypasses TypeScript's type checking. Consider using proper typing from the ControllerConnector package.- }) as never as Connector; + }) as Connector;Likely invalid or redundant comment.
1-1: Verify the new controller import path.The import path has changed from
@cartridge/connectorto@cartridge/connector/controller. Let's verify this is the correct import path for version 0.5.0.✅ Verification successful
Import path change is consistent across the codebase
The import path
@cartridge/connector/controlleris consistently used across multiple files in the UI codebase:
- ui/src/app/page.tsx
- ui/src/app/lib/connectors.ts
- ui/src/app/components/profile/ProfileDialog.tsx
This consistent usage pattern across different components suggests this is the correct import path for the updated controller version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the package structure and import path # Expected: Find evidence of the correct import path in package documentation or type definitions # Check if the package.json specifies the correct version echo "Checking package version:" jq -r '.dependencies["@cartridge/connector"]' package.json # Look for type definitions or documentation that confirms the import path echo "Checking for type definitions:" fd -e d.ts -e ts "controller.d.ts|index.d.ts" node_modules/@cartridge/connectorLength of output: 360
Script:
#!/bin/bash # Let's try a different approach to verify the import path # Check for any references to the controller import path in the codebase echo "Checking for controller import usage patterns:" rg -l "@cartridge/connector/controller" --type ts # Look for any migration guides or documentation echo "Checking documentation for migration info:" rg -A 5 "0.5.0|migration|upgrade" README.md # Check the actual structure of imports in the codebase echo "Analyzing import patterns:" ast-grep --pattern 'import $_ from "@cartridge/connector/$_"' # Look for package.json in the repository echo "Looking for package.json files:" fd package.jsonLength of output: 1481
ui/src/app/components/onboarding/Sections/WalletSection.tsx (1)
60-62: Consider UX impact of forced disconnectionThe current implementation forces a disconnect before connecting with a new connector. This might lead to a jarring user experience, especially if the user is already connected with the desired wallet.
Also applies to: 82-84
ui/src/app/components/profile/ProfileDialog.tsx (2)
Line range hint
121-141: Review security implications of withdrawal functionality.The withdrawal functionality has several security considerations that should be verified:
- The controller delegate validation only checks for "0x0" or empty string. Consider adding more robust validation.
- The withdrawal amount validation isn't visible in this component.
- The controller settings can be changed during an active withdrawal process.
10-10: Verify the continued usage of CartridgeIcon and checkCartridgeConnector.While the connector has been updated to use
ControllerConnector, the component still usesCartridgeIconandcheckCartridgeConnector. This might need to be updated for consistency with the new controller terminology.ui/src/app/page.tsx (1)
69-69: LGTM: Import updated to use ControllerConnectorThe import statement has been correctly updated to use the new ControllerConnector from the specific controller module path.
| disconnect(); | ||
| connect({ connector: cartridgeConnector }); | ||
| connect({ connector: controllerConnector }); | ||
| }} | ||
| > | ||
| Login with Cartridge Controller |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated connection logic
The connection handling logic is duplicated between mobile and desktop versions. Consider extracting this into a reusable function to improve maintainability.
+ const handleConnect = (connector: any) => {
+ disconnect();
+ connect({ connector });
+ };
+
// In desktop version
<Button
onClick={() => {
- disconnect();
- connect({ connector: controllerConnector });
+ handleConnect(controllerConnector);
}}
>
// In mobile version
<Button
onClick={() => {
- disconnect();
- connect({ connector: controllerConnector });
+ handleConnect(controllerConnector);
}}
>Also applies to: 82-86
| ( | ||
| connector as unknown as ControllerConnector | ||
| ).controller.openSettings() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety by avoiding unsafe type casting.
The current implementation uses an unsafe type cast with unknown. Consider using a type-safe approach:
- (
- connector as unknown as ControllerConnector
- ).controller.openSettings()
+ (connector as ControllerConnector).controller.openSettings()Better yet, consider updating the connector's type definition:
interface ConnectHookState {
connector: ControllerConnector | undefined;
// ... other properties
}
Summary by CodeRabbit
New Features
Bug Fixes
Withdrawbutton is conditionally enabled based on the connector's state.Documentation