-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: cosmwasmclient extension & signingcosmwasmclient implementation #379
Conversation
WalkthroughThe pull request introduces several modifications across multiple files. It updates the Jest configuration to exclude specific files from coverage collection, adds new dependencies in the Changes
Assessment against linked issues
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 (
|
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: 7
🧹 Outside diff range and nitpick comments (4)
src/sdk/core/cosmwasmclient.ts (1)
53-56
: Use specific error classes instead of genericError
Throwing a generic
Error
makes it difficult to handle specific error cases downstream. Consider using a more specific error class or creating a custom error to provide better context.src/sdk/core/signingcosmwasmclient.ts (3)
577-592
: Consider default gas multiplier for fee calculationIn the
signAndBroadcast
method, when calculating the fee with auto gas, you're usingthis.defaultGasMultiplier
. Ensure that this multiplier is appropriate for the transactions you are broadcasting. Adjusting it may improve the accuracy of gas estimation and transaction success rates.
707-713
: Simplify account retrieval from signerIn the
signAmino
method, the logic to retrieve the account from the signer can be streamlined for better readability.Apply this diff to simplify the account retrieval:
const accountFromSigner = (await this.signer.getAccounts()).find( (account) => account.address === signerAddress ) -if (!accountFromSigner) { - throw new Error("Failed to retrieve account from signer") -} +assert(accountFromSigner, "Failed to retrieve account from signer")
771-772
: Simplify public key encoding in direct signingIn the
signDirect
method, the encoding of the public key can be simplified for clarity.Apply this diff to simplify the public key encoding:
const pubkey = encodePubkey(encodeSecp256k1Pubkey(accountFromSigner.pubkey)) +const pubkey = encodePubkey({ + type: "tendermint/PubKeySecp256k1", + value: toBase64(accountFromSigner.pubkey), +})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (5)
- jest.config.ts (1 hunks)
- package.json (2 hunks)
- src/sdk/core/cosmwasmclient.ts (1 hunks)
- src/sdk/core/signingcosmwasmclient.ts (1 hunks)
- src/sdk/tx/txClient.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (15)
jest.config.ts (1)
25-26
: LGTM: Appropriate exclusions for external implementations.The addition of these two lines to exclude
cosmwasmclient.ts
andsigningcosmwasmclient.ts
from coverage collection is appropriate. These files are implementations from Cosmjs, as indicated by the comments, and excluding them from coverage allows the project to focus on its custom extensions and implementations.This change aligns well with the PR objectives, particularly the implementation of a new
SigningCosmWasmClient
. By excluding the base implementation from coverage, it ensures that the coverage reports will more accurately reflect the custom work done in this project.package.json (2)
50-50
: LGTM: Addition ofpako
dependency is justified.The addition of the
pako
package (version ^2.1.0) is appropriate for the implementation ofSigningCosmWasmClient
. This addresses the previous comment about unused packages, as it provides necessary functionality for compression/decompression operations likely used in the new client implementation.
71-71
: LGTM: Addition of@types/pako
dev dependency.The inclusion of
@types/pako
(version ^2.0.3) as a dev dependency is a good practice. It provides TypeScript type definitions for thepako
package, ensuring type safety and improving developer experience with better IDE support.src/sdk/core/cosmwasmclient.ts (2)
18-22
: Ensure correct parameters are passed to thesuper
constructorVerify that passing
cometClient
to thesuper
constructor ofCosmWasmClient
is appropriate. The base class may expect different parameters or initialization logic. Consult theCosmWasmClient
constructor documentation to ensure compatibility.
9-9
: Verify ifCometClient
is the correct client to useEnsure that
CometClient
is the appropriate client class imported from@cosmjs/tendermint-rpc
. Depending on the version, the client might be named differently (e.g.,TmClient
orTendermintClient
). Confirm thatCometClient
is correctly imported and compatible withCosmWasmClient
.Run the following script to check for the correct import of
CometClient
:src/sdk/tx/txClient.ts (8)
15-15
: ImportingsetupWasmExtension
The addition of
setupWasmExtension
from"@cosmjs/cosmwasm-stargate"
ensures that the WASM functionalities are extended properly in the client.
18-21
: Importing Nibiru-specific Signing ClientsThe import statements for
NibiSigningCosmWasmClient
andNibiSigningCosmWasmClientOptions
from"../core/signingcosmwasmclient"
correctly bring in the Nibiru-specific implementations needed for customized account handling.
29-29
: UpdatingwasmClient
to Nibiru ImplementationThe
wasmClient
property type has been updated toNibiSigningCosmWasmClient
, aligning with the shift to the Nibiru-specific signing client. This ensures compatibility with the custom account parser and addresses the issues outlined in the PR objectives.
35-35
: Constructor Parameter UpdateThe constructor now accepts a
wasm
parameter of typeNibiSigningCosmWasmClient
. This change is consistent with the updatedwasmClient
property and supports the new client implementation.
55-55
: Updating WASM Options TypeThe
wasmOptions
parameter type inconnectWithSigner
has been updated toNibiSigningCosmWasmClientOptions
, which is appropriate for the Nibiru-specific client configuration.
58-62
: Connecting with Nibiru Signing CosmWasm ClientThe method
NibiSigningCosmWasmClient.connectWithSigner
is correctly used to create a new instance of the signing client. This replaces the standard CosmWasm client with the Nibiru-specific implementation.
63-63
: Ensuring Override Order in Options SpreadIn the options object passed to
NibiSigningCosmWasmClient.connectWithSigner
, the spread operator...wasmOptions
comes after the defaultaccountParser
andgasPrice
. This means ifwasmOptions
containsaccountParser
orgasPrice
, it will override the default settings. Ensure this behavior is intentional.
63-63
: Consistent Account Parser ConfigurationThe addition of
accountParser: accountFromNibiru
ensures that the custom account parser is used, addressing the issue of handling Ethereum account types. This change is crucial for resolving the errors encountered with new Keplr wallets.src/sdk/core/signingcosmwasmclient.ts (2)
715-716
: Ensure correctsignMode
is used in Amino signingIn the
signAmino
method, verify that thesignMode
used matches the intended signing mode for Amino transactions. Using an incorrectsignMode
can lead to signature verification failures.Check that
SignMode.SIGN_MODE_LEGACY_AMINO_JSON
is appropriate for your use case and compatible with the network's requirements.
576-589
:⚠️ Potential issueEnsure
gasPrice
is defined when using auto gas calculationIn the
signAndBroadcast
method, whenfee
is set to"auto"
or a number, you assert thatthis.gasPrice
is defined. Ifthis.gasPrice
isundefined
, an error will be thrown. To improve user experience, consider providing a more descriptive error message or setting a defaultgasPrice
.Run the following script to verify if instances of
NibiSigningCosmWasmClient
always havegasPrice
set when auto gas is used:✅ Verification successful
Let's re-run the verification with the corrected scripts to ensure
gasPrice
is properly defined when using auto gas.
gasPrice
is consistently defined when using auto gas calculation. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if NibiSigningCosmWasmClient instances have gasPrice defined when auto gas is used # Test: Search for constructor calls without gasPrice. Expect: No matches. rg --type ts 'new NibiSigningCosmWasmClient\((.*), (.*), \{(?!.*gasPrice)' -A 5 # Test: Search for signAndBroadcast calls with 'auto' or number fee without gasPrice. Expect: No matches. rg --type ts 'signAndBroadcast\(([^,]+), ([^,]+), (("auto")|(\d+)), [^,]+\)' -A 5Length of output: 473
Script:
#!/bin/bash # Description: Check if NibiSigningCosmWasmClient instances have gasPrice defined when auto gas is used # Test: Search for constructor calls without gasPrice. Expect: No matches. rg --type ts --pcre2 'new NibiSigningCosmWasmClient\((.*), (.*), \{(?!.*gasPrice)' -A 5 # Test: Search for signAndBroadcast calls with "auto" or number fee without gasPrice. Expect: No matches. rg --type ts --pcre2 'signAndBroadcast\(([^,]+), ([^,]+), ("auto"|\d+), [^,]+\)' -A 5Length of output: 177
…380) * revert: cosmos submodule only (#362) * revert: cosmos submodule only * fix: rem * fix: rem * fix: update * feat: add msg client * fix: paths * fix: try chaosnet ibc * fix: path again * fix: try hm * fix: fixes to pass * feat: eth protos (#366) * fix: eth protos * fix: client * fix: fixes * fix: try older nibiru * fix: index * fix: mainnet * fix: import * revert: build change * chore: tests (#367) * fix: all query tests * chore: final tests * fix: buf * fix: fix * fix: pull latest * fix: build * fix: build * refactor(faucet)!: set default tokens (#369) * chore: develop -> main (#368) * revert: cosmos submodule only (#362) * revert: cosmos submodule only * fix: rem * fix: rem * fix: update * feat: add msg client * fix: paths * fix: try chaosnet ibc * fix: path again * fix: try hm * fix: fixes to pass * feat: eth protos (#366) * fix: eth protos * fix: client * fix: fixes * fix: try older nibiru * fix: index * fix: mainnet * fix: import * revert: build change * chore: tests (#367) * fix: all query tests * chore: final tests * fix: buf * fix: fix * fix: pull latest * fix: build * fix: build * chore(release): 4.5.1 ### [4.5.1](v4.5.0...v4.5.1) (2024-08-09) ### Miscellaneous Chores * develop -> main ([#368](#368)) ([c6d6570](c6d6570)), closes [#362](#362) [#366](#366) [#367](#367) [skip ci] * fix(faucet): remove unused tokens from default faucet request * fix: bump test --------- Co-authored-by: Cameron Gilbert <[email protected]> Co-authored-by: semantic-release-bot <[email protected]> * chore: main to develop (#375) * chore: develop -> main (#370) * revert: cosmos submodule only (#362) * revert: cosmos submodule only * fix: rem * fix: rem * fix: update * feat: add msg client * fix: paths * fix: try chaosnet ibc * fix: path again * fix: try hm * fix: fixes to pass * feat: eth protos (#366) * fix: eth protos * fix: client * fix: fixes * fix: try older nibiru * fix: index * fix: mainnet * fix: import * revert: build change * chore: tests (#367) * fix: all query tests * chore: final tests * fix: buf * fix: fix * fix: pull latest * fix: build * fix: build * refactor(faucet)!: set default tokens (#369) * chore: develop -> main (#368) * revert: cosmos submodule only (#362) * revert: cosmos submodule only * fix: rem * fix: rem * fix: update * feat: add msg client * fix: paths * fix: try chaosnet ibc * fix: path again * fix: try hm * fix: fixes to pass * feat: eth protos (#366) * fix: eth protos * fix: client * fix: fixes * fix: try older nibiru * fix: index * fix: mainnet * fix: import * revert: build change * chore: tests (#367) * fix: all query tests * chore: final tests * fix: buf * fix: fix * fix: pull latest * fix: build * fix: build * chore(release): 4.5.1 ### [4.5.1](v4.5.0...v4.5.1) (2024-08-09) ### Miscellaneous Chores * develop -> main ([#368](#368)) ([c6d6570](c6d6570)), closes [#362](#362) [#366](#366) [#367](#367) [skip ci] * fix(faucet): remove unused tokens from default faucet request * fix: bump test --------- Co-authored-by: Cameron Gilbert <[email protected]> Co-authored-by: semantic-release-bot <[email protected]> --------- Co-authored-by: Kevin Yang <[email protected]> Co-authored-by: semantic-release-bot <[email protected]> * chore(github): Add project automation for https://tinyurl.com/25uty9w5 * chore(release): 4.5.2 ### [4.5.2](v4.5.1...v4.5.2) (2024-09-24) ### Miscellaneous Chores * develop -> main ([#370](#370)) ([ec2a25b](ec2a25b)), closes [#362](#362) [#366](#366) [#367](#367) [#369](#369) [#368](#368) [#362](#362) [#366](#366) [#367](#367) [#362](#362) [#366](#366) [#367](#367) * **github:** Add project automation for https://tinyurl.com/25uty9w5 ([c2c27e5](c2c27e5)) [skip ci] --------- Co-authored-by: Cameron Gilbert <[email protected]> Co-authored-by: Kevin Yang <[email protected]> Co-authored-by: semantic-release-bot <[email protected]> Co-authored-by: Unique Divine <[email protected]> * feat: account parser (#374) * chore: develop -> main (#370) * revert: cosmos submodule only (#362) * revert: cosmos submodule only * fix: rem * fix: rem * fix: update * feat: add msg client * fix: paths * fix: try chaosnet ibc * fix: path again * fix: try hm * fix: fixes to pass * feat: eth protos (#366) * fix: eth protos * fix: client * fix: fixes * fix: try older nibiru * fix: index * fix: mainnet * fix: import * revert: build change * chore: tests (#367) * fix: all query tests * chore: final tests * fix: buf * fix: fix * fix: pull latest * fix: build * fix: build * refactor(faucet)!: set default tokens (#369) * chore: develop -> main (#368) * revert: cosmos submodule only (#362) * revert: cosmos submodule only * fix: rem * fix: rem * fix: update * feat: add msg client * fix: paths * fix: try chaosnet ibc * fix: path again * fix: try hm * fix: fixes to pass * feat: eth protos (#366) * fix: eth protos * fix: client * fix: fixes * fix: try older nibiru * fix: index * fix: mainnet * fix: import * revert: build change * chore: tests (#367) * fix: all query tests * chore: final tests * fix: buf * fix: fix * fix: pull latest * fix: build * fix: build * chore(release): 4.5.1 ### [4.5.1](v4.5.0...v4.5.1) (2024-08-09) ### Miscellaneous Chores * develop -> main ([#368](#368)) ([c6d6570](c6d6570)), closes [#362](#362) [#366](#366) [#367](#367) [skip ci] * fix(faucet): remove unused tokens from default faucet request * fix: bump test --------- Co-authored-by: Cameron Gilbert <[email protected]> Co-authored-by: semantic-release-bot <[email protected]> --------- Co-authored-by: Kevin Yang <[email protected]> Co-authored-by: semantic-release-bot <[email protected]> * chore(github): Add project automation for https://tinyurl.com/25uty9w5 * chore(release): 4.5.2 ### [4.5.2](v4.5.1...v4.5.2) (2024-09-24) ### Miscellaneous Chores * develop -> main ([#370](#370)) ([ec2a25b](ec2a25b)), closes [#362](#362) [#366](#366) [#367](#367) [#369](#369) [#368](#368) [#362](#362) [#366](#366) [#367](#367) [#362](#362) [#366](#366) [#367](#367) * **github:** Add project automation for https://tinyurl.com/25uty9w5 ([c2c27e5](c2c27e5)) [skip ci] * feat: nibiru account parser * refactor: throw if baseaccount is undefined * test: fixing tests * chore: removing unnecessary ? * refactor: matching cosmjs implementation * chore: removing t.json * chore: pr comments --------- Co-authored-by: Cameron Gilbert <[email protected]> Co-authored-by: Kevin Yang <[email protected]> Co-authored-by: semantic-release-bot <[email protected]> Co-authored-by: Unique Divine <[email protected]> * chore: remove stats and update default feature flags * feat: cosmwasmclient extension & signingcosmwasmclient implementation (#379) * feat: nibicosmwasmclient * feat: nibi signing cosm wasm client * refactor: adding nibi account parser to nibi signingcosmwasmclient * test: remove unused test file * test: take signingcosmwasmclient from coverage * chore: fix coverage --------- Co-authored-by: Kevin Yang <[email protected]> Co-authored-by: semantic-release-bot <[email protected]> Co-authored-by: Calico Nino <[email protected]> Co-authored-by: Unique Divine <[email protected]>
## [5.0.0](v4.5.2...v5.0.0) (2024-10-18) ### ⚠ BREAKING CHANGES * **faucet:** set default tokens (#369) ### Features * account parser ([#374](#374)) ([d7e324d](d7e324d)), closes [#370](#370) [#362](#362) [#366](#366) [#367](#367) [#369](#369) [#368](#368) [#362](#362) [#366](#366) [#367](#367) [#362](#362) [#366](#366) [#367](#367) * add msg client ([32374dd](32374dd)) * cosmwasmclient extension & signingcosmwasmclient implementation ([#379](#379)) ([b89a700](b89a700)) * eth protos ([#366](#366)) ([9d496c7](9d496c7)), closes [#367](#367) ### Bug Fixes * build ([b9b3d74](b9b3d74)) * build ([79fcd49](79fcd49)) * fixes to pass ([0c3f86a](0c3f86a)) * path again ([e5f1ff3](e5f1ff3)) * paths ([c762665](c762665)) * pull latest ([35eb2d8](35eb2d8)) * try chaosnet ibc ([d66c15c](d66c15c)) * try hm ([954f259](954f259)) ### Reverts * cosmos submodule only ([#362](#362)) ([1d09461](1d09461)) ### Code Refactors * **faucet:** set default tokens ([#369](#369)) ([859882a](859882a)), closes [#368](#368) [#362](#362) [#366](#366) [#367](#367) [#362](#362) [#366](#366) [#367](#367) ### Miscellaneous Chores * develop -> main ([5518ddd](5518ddd)) * develop -> main (remove stats + update default feature flags) ([#380](#380)) ([087d569](087d569)), closes [#362](#362) [#366](#366) [#367](#367) [#369](#369) [#368](#368) [#362](#362) [#366](#366) [#367](#367) [#362](#362) [#366](#366) [#367](#367) * main to develop ([#375](#375)) ([49b0766](49b0766)), closes [#370](#370) [#362](#362) [#366](#366) [#367](#367) [#369](#369) [#368](#368) [#362](#362) [#366](#366) [#367](#367) [#362](#362) [#366](#366) [#367](#367) * remove stats and update default feature flags ([56bef2a](56bef2a)) * trigger release ([d89c408](d89c408)) [skip ci]
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Unsupported type: '/eth.types.v1.EthAccount'
with brand new keplr wallet #371Mitigating cosmos/cosmjs#1610 in NibiJs.
Summary by CodeRabbit
New Features
NibiCosmWasmClient
andNibiSigningCosmWasmClient
classes for enhanced interaction with CosmWasm smart contracts, including account management and transaction handling.Dependencies
pako
and@types/pako
as new dependencies in the project.Configuration Updates