-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: remove wallet restriction on getCosmosWallet #531
chore: remove wallet restriction on getCosmosWallet #531
Conversation
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/wallets/wallet-core/src/broadcaster/MsgBroadcaster.tsOops! Something went wrong! :( ESLint: 7.32.0 Error: Error while loading rule 'jest/unbound-method': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser. WalkthroughThe pull request introduces modifications to the wallet core functionality, specifically in the Changes
Sequence DiagramsequenceDiagram
participant Wallet
participant MsgBroadcaster
participant Strategy
Wallet->>Strategy: Request Cosmos Wallet
Strategy-->>Wallet: Return Cosmos Wallet
Wallet->>MsgBroadcaster: Broadcast Transaction
MsgBroadcaster->>MsgBroadcaster: Check Gas Conditions
MsgBroadcaster-->>Wallet: Broadcast Result
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/wallets/wallet-core/src/broadcaster/MsgBroadcaster.ts (3)
841-843
: Validate or centralize wallet conditional checks.Currently, you check if the wallet is either Keplr or OWallet to set canDisableCosmosGasCheck. For future scalability, consider centralizing this condition in a helper function or a configuration map, so you can easily manage additional wallet types in one place without scattering logic across the codebase.
894-894
: Guard against repeated disableGasCheck calls.The condition checks both canDisableCosmosGasCheck and whether cosmosWallet.disableGasCheck is defined. As a minor enhancement, ensure you handle scenarios where disabling the gas check might already be in effect or might fail. Logging or handling potential errors can improve observability.
919-919
: Ensure consistent re-enabling logic.After disabling the gas check, re-enabling is essential, but consider confirming or logging that the gas check was indeed disabled before calling enableGasCheck. This can safeguard against unexpected states if new wallet features or behaviors are introduced in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/wallets/wallet-core/src/broadcaster/MsgBroadcaster.ts
(3 hunks)packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts
(0 hunks)
💤 Files with no reviewable changes (1)
- packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts
Summary by CodeRabbit
New Features
getCosmosWallet
method, allowing use with any supported wallet type.Bug Fixes