fix: update wallet_app tutorial #431
Conversation
|
To view this pull requests documentation preview, visit the following URL: docs.page/focustree/starknet.dart~431 Documentation is deployed and generated using docs.page. |
|
Warning Rate limit exceeded@ptisserand has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 40 minutes and 24 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 (1)
WalkthroughThe pull request introduces several modifications to the mobile wallet application's tutorial and functionality. Key updates include changing the Changes
Sequence DiagramsequenceDiagram
participant User
participant UI
participant WalletProvider
participant WalletService
participant SecureStore
User->>UI: Select Account
UI->>WalletProvider: deployAccount()
WalletProvider->>WalletService: deployAccount()
WalletService->>SecureStore: Retrieve Private Key
WalletService->>WalletService: Create Deployment Transaction
WalletService-->>WalletProvider: Deployment Result
WalletProvider-->>UI: Update Account Status
UI->>User: Show Deployment Confirmation
Possibly related PRs
Poem
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 (
|
…or account balances keys
…bile-wallet tutorial
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/wallet_kit/lib/widgets/wallet_list.dart (3)
139-156: Consider optimizing balance refresh for multiple accounts.While the implementation correctly refreshes balances on expansion, consider batching the balance refresh operations when dealing with multiple accounts to avoid potential performance issues.
- wallet.accounts.forEach((key, value) { - ref.read(walletsProvider.notifier) - ..refreshEthBalance(value.walletId, value.id) - ..refreshStrkBalance(value.walletId, value.id); - }); + // Batch refresh balances + final accounts = wallet.accounts.values.toList(); + ref.read(walletsProvider.notifier).batchRefreshBalances(accounts);Also consider adding error handling for failed balance refresh operations.
234-234: Enhance accessibility for deployment status indication.While the color-based status indication is implemented correctly, consider adding additional visual cues for better accessibility:
final isDeployed = account.isDeployed; return FilledButton.tonal( style: ButtonStyle( backgroundColor: WidgetStateProperty.all(isDeployed ? Colors.white : Colors.grey.shade400.withValues(alpha: 0.5)), + // Add an icon to indicate deployment status + leading: Icon( + isDeployed ? Icons.check_circle : Icons.pending, + semanticLabel: isDeployed ? 'Deployed' : 'Not deployed', + ),Also applies to: 240-242
273-274: Improve balance display formatting.Consider formatting the balance value for better readability, especially for large numbers or decimal places.
-Text('${(account.balances[TokenSymbol.ETH.name] ?? 0).toString()} ETH'), +Text('${formatBalance(account.balances[TokenSymbol.ETH.name] ?? 0)} ETH'),Where
formatBalancecould be a utility function that handles:
- Rounding to appropriate decimal places
- Adding thousand separators
- Handling very small/large numbers
packages/wallet_kit/lib/widgets/deploy_account_button.dart (2)
20-20: Fix typo in button labelThere's a typo in the button label: "Not enougth ETH" should be "Not enough ETH".
- label: enoughBalance ? 'Deploy account' : 'Not enougth ETH', + label: enoughBalance ? 'Deploy account' : 'Not enough ETH',
16-18: Extract minimum ETH balance as a constantThe minimum ETH balance (0.00001) should be extracted as a named constant for better maintainability and reusability.
+ static const double MINIMUM_ETH_BALANCE = 0.00001; + @override Widget build(BuildContext context, WidgetRef ref) { // ... if (selectedAccount?.isDeployed == false) { final ethBalance = selectedAccount!.balances[TokenSymbol.ETH.name] ?? 0.00; - final enoughBalance = ethBalance >= 0.00001; + final enoughBalance = ethBalance >= MINIMUM_ETH_BALANCE;docs/examples/mobile-wallet.mdx (3)
26-33: Add explanation for ACCOUNT_CLASS_HASH update.Good addition of the note about matching ACCOUNT_CLASS_HASH with starknet-devnet version. Consider adding:
- Why the hash changed
- Impact of using mismatched hashes
256-265: Fix grammar and enhance minting instructions.
- Grammar: "Deploying an account require" should be "Deploying an account requires"
- Consider adding:
- Explanation of the amount parameter (20000000000000000000 WEI = 20 ETH)
- Warning about using example address
🧰 Tools
🪛 LanguageTool
[uncategorized] ~256-~256: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...n action! 💸 --- Deploying an account require some ETH to pay transaction fees. With ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
60-68: Clarify biometric support instructions.The biometric support section could be clearer:
- Specify what biometric features are supported (fingerprint, face recognition, etc.)
- Add error handling recommendations
- Explain the reason for using FlutterFragmentActivity
🧰 Tools
🪛 LanguageTool
[style] ~61-~61: Consider a shorter alternative to avoid wordiness.
Context: ...} ``` 7. Biometric support (optional) In order to useBiometricon Android, your `MainA...(IN_ORDER_TO_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/examples/mobile-wallet.mdx(3 hunks)packages/wallet_kit/lib/services/wallet_service.dart(2 hunks)packages/wallet_kit/lib/wallet_state/wallet_provider.dart(3 hunks)packages/wallet_kit/lib/widgets/deploy_account_button.dart(1 hunks)packages/wallet_kit/lib/widgets/index.dart(1 hunks)packages/wallet_kit/lib/widgets/send_eth_button.dart(1 hunks)packages/wallet_kit/lib/widgets/token_list.dart(2 hunks)packages/wallet_kit/lib/widgets/wallet_list.dart(3 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/examples/mobile-wallet.mdx
[style] ~61-~61: Consider a shorter alternative to avoid wordiness.
Context: ...} ``` 7. Biometric support (optional) In order to use Biometric on Android, your `MainA...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~256-~256: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...n action! 💸 --- Deploying an account require some ETH to pay transaction fees. With ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🔇 Additional comments (8)
packages/wallet_kit/lib/widgets/wallet_list.dart (1)
133-137: LGTM! Clean controller lifecycle management.The controller initialization and cleanup is properly handled using the useEffect hook.
packages/wallet_kit/lib/widgets/index.dart (1)
11-11: LGTM!The export for
deploy_account_button.dartis correctly added, making the newDeployAccountButtonwidget accessible to other parts of the application.packages/wallet_kit/lib/widgets/send_eth_button.dart (1)
19-21: LGTM!Good addition of the null check to gracefully handle cases when no account is selected.
packages/wallet_kit/lib/widgets/token_list.dart (2)
17-25: LGTM!Good implementation of balance refresh logic using the cascade operator to refresh both ETH and STRK balances efficiently.
52-52: LGTM!Good update to use
symbol.namefor accessing balances, ensuring consistent token symbol handling.packages/wallet_kit/lib/services/wallet_service.dart (1)
166-166: Good security practice: nullifying the signer.Setting the signer to null after use is a good security practice to avoid keeping sensitive data in memory.
packages/wallet_kit/lib/wallet_state/wallet_provider.dart (2)
131-142: LGTM: Clean implementation of account deployment.Good separation of concerns between the service call and state management. The implementation correctly updates the deployment status after the operation.
214-214: Good practice: Using TokenSymbol enum instead of string literals.Using
TokenSymbol.ETH.nameinstead of string literals improves type safety and maintainability.
| final chainId = WalletKit().chainId; | ||
|
|
||
| final fundingAccount = s.Account( | ||
| provider: provider, | ||
| signer: signer, | ||
| accountAddress: s.Felt.fromHexString(account.address), | ||
| chainId: s.StarknetChainId.testNet, | ||
| chainId: chainId, |
There was a problem hiding this comment.
Remove hardcoded private key.
Good change to use dynamic chainId from WalletKit. However, there's a hardcoded private key (0x1) which should be removed from production code.
The commented-out code suggests using PasswordStore - consider implementing this instead of the hardcoded value.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/wallet_kit/lib/widgets/wallet_list.dart (2)
133-156: Add loading state and optimize balance refreshThe current implementation refreshes all accounts on expansion without visual feedback.
Consider these improvements:
- Add loading indicators during refresh
- Debounce or throttle refresh operations
- Only refresh visible accounts
Apply this diff as a starting point:
useEffect(() { if (controller.value != null) { + final debouncer = Debouncer(milliseconds: 500); void onExpanded() { if (controller.value!.expanded) { + ref.read(loadingProvider.notifier).state = true; + debouncer.run(() async { wallet.accounts.forEach( (key, value) => ref .read(walletsProvider.notifier) .refreshAccount(value.walletId, value.id), ); + ref.read(loadingProvider.notifier).state = false; + }); } }
234-242: Add accessibility support for deployment statusWhile the visual feedback is good, it should be complemented with semantic labels for screen readers.
Apply this diff to improve accessibility:
return FilledButton.tonal( + semanticsLabel: '${account.name}, ${isDeployed ? 'deployed' : 'not deployed'} account with ${account.balances[TokenSymbol.ETH.name] ?? 0} ETH', style: ButtonStyle( backgroundColor: WidgetStateProperty.all(isDeployed ? Colors.white : Colors.grey.shade400.withValues(alpha: 0.5)),Also applies to: 273-274
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/wallet_kit/lib/services/wallet_service.dart(2 hunks)packages/wallet_kit/lib/wallet_state/wallet_provider.dart(4 hunks)packages/wallet_kit/lib/widgets/wallet_list.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wallet_kit/lib/services/wallet_service.dart
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-integration
- GitHub Check: analyze
- GitHub Check: lint
🔇 Additional comments (1)
packages/wallet_kit/lib/wallet_state/wallet_provider.dart (1)
227-227: LGTM! Good use of TokenSymbol enumThe change from hardcoded strings to enum references improves maintainability and type safety.
Also applies to: 262-263
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/wallet_kit/lib/wallet_state/wallet_provider.dart (1)
133-144:⚠️ Potential issueAdd error handling and return type for deployAccount method.
The method should handle deployment failures gracefully and maintain consistent state.
Apply this diff to improve error handling:
- deployAccount({ + Future<bool> deployAccount({ required SecureStore secureStore, required Account account, }) async { + try { final success = await WalletService.deployAccount( secureStore: secureStore, account: account); updateSelectedAccountIsDeployed( walletId: account.walletId, accountId: account.id, isDeployed: success, ); + return success; + } catch (e) { + updateSelectedAccountIsDeployed( + walletId: account.walletId, + accountId: account.id, + isDeployed: false, + ); + rethrow; + } }
🧹 Nitpick comments (4)
packages/wallet_kit/lib/wallet_state/wallet_provider.dart (1)
189-206: Add return type annotation for clarity.The method has good error handling and concurrency control, but would benefit from an explicit return type.
- Future<void> refreshAccount(String walletId, int accountId) async { + Future<void> refreshAccount(String walletId, int accountId) async {docs/examples/mobile-wallet.mdx (3)
60-68: Fix typo and enhance biometric support explanation.
- There's a typo in the file extension: "MainActivity.tk" should be "MainActivity.kt"
- Consider adding a brief explanation of why FlutterFragmentActivity is required (it provides the necessary support for biometric authentication dialogs)
-You need to modify your `MainActivity.tk` with: +You need to modify your `MainActivity.kt` with:🧰 Tools
🪛 LanguageTool
[style] ~61-~61: Consider a shorter alternative to avoid wordiness.
Context: ...} ``` 7. Biometric support (optional) In order to useBiometricon Android, your `MainA...(IN_ORDER_TO_PREMIUM)
209-226: Add documentation for Layout2 widget.The HomeScreen implementation looks clean, but it would be helpful to add a brief explanation of the
Layout2widget from wallet_kit, including its purpose and layout structure.
256-265: Improve minting example with placeholder address.The minting instructions are helpful, but consider replacing the hardcoded address with a placeholder to make it clear that users should replace it with their own address.
- -d '{"address": "0x5461cf5be91b3f21304ad81a3d0efeb5fe95f9655d7353ab0fc822e78b10837", "amount": 20000000000000000000, "unit": "WEI"}' + -d '{"address": "<YOUR_ACCOUNT_ADDRESS>", "amount": 20000000000000000000, "unit": "WEI"}'Also consider adding a note explaining how to obtain your account address from the wallet interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/examples/mobile-wallet.mdx(3 hunks)packages/wallet_kit/lib/wallet_state/wallet_provider.dart(5 hunks)packages/wallet_kit/lib/widgets/deploy_account_button.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wallet_kit/lib/widgets/deploy_account_button.dart
🧰 Additional context used
🪛 LanguageTool
docs/examples/mobile-wallet.mdx
[style] ~61-~61: Consider a shorter alternative to avoid wordiness.
Context: ...} ``` 7. Biometric support (optional) In order to use Biometric on Android, your `MainA...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: analyze
- GitHub Check: test-integration
- GitHub Check: lint
🔇 Additional comments (5)
packages/wallet_kit/lib/wallet_state/wallet_provider.dart (4)
16-17: LGTM: Good practice to prevent concurrent refreshes.The private boolean field is well-named and appropriately used to prevent race conditions during balance refreshes.
235-235: LGTM: Good use of enum for token symbol.Using
TokenSymbol.ETH.nameinstead of a string literal improves maintainability and type safety.
270-271: LGTM: Consistent use of enum for token symbol.Using
TokenSymbol.STRK.namemaintains consistency with the ETH balance update and improves code quality.
235-235: Verify token symbol usage across the codebase.Let's ensure the switch to using
TokenSymbolenum is consistent across the codebase.Also applies to: 270-271
✅ Verification successful
TokenSymbol enum usage is consistent across the codebase
The codebase consistently uses the TokenSymbol enum for both ETH and STRK tokens, with no direct string literals found except in documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining string literals for token symbols # that should be using the TokenSymbol enum echo "Searching for ETH string literals..." rg -l "'ETH'|\"ETH\"" echo "Searching for STRK string literals..." rg -l "'STRK'|\"STRK\"" echo "Checking current usage of TokenSymbol enum..." rg "TokenSymbol\."Length of output: 955
docs/examples/mobile-wallet.mdx (1)
44-58: LGTM! Clear instructions for Android configuration.The documentation clearly explains the minimum SDK requirement and provides the exact configuration needed in build.gradle. This is essential for the secure_store functionality.
secure_storerequire Android minimum SDK version set to at least 23Disable deploy buttonwhen ETH balance is not enoughSummary by CodeRabbit
Release Notes
New Features
DeployAccountButtonfor deploying undeployed accounts.Improvements
Documentation
Compatibility
ACCOUNT_CLASS_HASHforstarknet-devnet.