Release/v1.54.0 #258
Release/v1.54.0 #258
Conversation
…v1_51_1 Feat/sync dec with master v1 51 1
See client/keyring/README.md for more details.
This PR removes a redundant error handling block in the `authz_vote.go` file to improve code clarity and reduce unnecessary code duplication. Signed-off-by: Hwangjae Lee <[email protected]>
(feat) Add OFAC list check
feat: new cosmos keyring helper for injective client
See client/keyring/README.md for more details.
…x/sync_dev_after_v1_52_0
Fix/sync dev after v1.52.0
Fix/sync dev after v1.52.1
Remove duplicate error handling in authz_vote.go
feat: add GetStatus to tmclient
…the TX in the mainpool if it has been rejected by the node during broadcast
Fix/sync dev after v1 52 2
…ps to be able to read/download the file
…ved the test to use a temp test directory
Fix/sync dev after v1 53 0
…om chain instead of querying Indexer. The tokens info is exclusively taken from Injective Lists. Users can also initialize tokens form the chain Token Denoms
… feat/sync_dev_with_v1_53_1
Feat/sync dev with v1.53.1
…om chain instead of querying Indexer. The tokens info is exclusively taken from Injective Lists. Users can also initialize tokens form the chain Token Denoms
…/github.com/InjectiveLabs/sdk-go into feat/refactor_markets_tokens_initialization
…ns_initialization Feat/refactor markets tokens initialization
WalkthroughThe pull request introduces several enhancements to the codebase, including modifications to the GitHub Actions workflow for testing, updates to the keyring management system, and the introduction of a new Changes
Possibly related PRs
Suggested reviewers
Poem
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. 🔧 golangci-lintlevel=warning msg="[lintersdb] The name "gas" is deprecated. The linter has been renamed to: gosec." 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (71)
.pre-commit-config.yaml (1)
Line range hint
11-14: Remove duplicateend-of-file-fixerhook entryThere's a duplicate entry for the
end-of-file-fixerhook which should be removed to avoid redundant checks.Apply this diff to fix the duplicate entry:
- id: trailing-whitespace - id: end-of-file-fixer - - id: end-of-file-fixer - id: check-yaml.github/workflows/run-tests.yml (2)
18-19: Consider pinning the pass package versionWhile the installation works, consider pinning the package version for better reproducibility across workflow runs.
- run: sudo apt-get update && sudo apt-get install -y pass + run: sudo apt-get update && sudo apt-get install -y pass=1.7.4-5
18-24: Consider creating a reusable composite action for keyring setupThe current implementation adds multiple steps directly in the workflow. This setup might be needed in other workflows and would benefit from being extracted into a reusable composite action with proper documentation.
Consider:
- Creating a composite action in
.github/actions/setup-keyring/- Adding documentation explaining why these steps are necessary
- Making the key configuration parameterizable
Example structure:
# .github/actions/setup-keyring/action.yml name: 'Setup Keyring' description: 'Sets up GPG and pass for keyring testing' inputs: key-name: description: 'Name for the GPG key' required: false default: 'keyring_test' runs: using: 'composite' steps: # Current steps would go hereThen use it in the workflow:
- uses: ./.github/actions/setup-keyringclient/keyring/errors.go (1)
5-20: Consider adding godoc comments and more specific error messages.While the implementation is solid, consider these improvements:
- Add godoc comments for each exported error variable to provide more context about when these errors occur.
- Make error messages more specific about what exactly failed, especially for errors like
ErrDeriveFailedandErrPrivkeyConflict.Example improvement:
var ( + // ErrCosmosKeyringCreationFailed is returned when the keyring cannot be created + // due to backend initialization failures or invalid configuration ErrCosmosKeyringCreationFailed = errors.New("cosmos keyring creation failed") + // ErrCosmosKeyringImportFailed is returned when attempting to import an existing key + // into the keyring fails due to format or permission issues ErrCosmosKeyringImportFailed = errors.New("cosmos keyring unable to import key") - ErrDeriveFailed = errors.New("key derivation failed") + ErrDeriveFailed = errors.New("failed to derive key from provided seed or path") - ErrPrivkeyConflict = errors.New("privkey conflict") + ErrPrivkeyConflict = errors.New("private key already exists with different parameters")client/keyring/key_config.go (1)
8-14: Add field documentation and security considerations.The
cosmosKeyConfigstruct handles sensitive information. Consider these improvements:
- Add documentation for each field explaining its purpose and constraints
- Consider adding struct tags for validation
- Mark sensitive fields (PrivKeyHex, Mnemonic, KeyPassphrase) for secure handling (e.g., using
sensitivetags or wrapper types that implement secure string handling)type cosmosKeyConfig struct { + // Name represents the identifier for this key configuration Name string + // KeyFrom specifies the key name to use for signing KeyFrom string + // KeyPassphrase is the passphrase for keyring files (sensitive) + // @sensitive KeyPassphrase string + // PrivKeyHex is the hexadecimal representation of the private key (sensitive) + // @sensitive PrivKeyHex string + // Mnemonic is the BIP39 mnemonic phrase for key derivation (sensitive) + // @sensitive Mnemonic string }examples/exchange/oracle/2_Price/example.go (1)
19-22: Consider improving error handling and endpoint configuration.While the initialization is correct, consider these improvements:
- Implement more graceful error handling instead of panic
- Make the websocket path configurable through the network configuration
- tmClient, err := rpchttp.New(network.TmEndpoint, "/websocket") - if err != nil { - panic(err) - } + tmClient, err := rpchttp.New(network.TmEndpoint, network.WebsocketPath) + if err != nil { + return fmt.Errorf("failed to initialize RPC client: %w", err) + }examples/chain/exchange/25_MsgUpdateDerivativeMarket/example.go (1)
63-65: Add documentation for the hardcoded decimal value.While the hardcoded decimal value of 6 is correct for USDT, adding a comment would improve code clarity.
+ // USDT has 6 decimals in the Injective ecosystem chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6))) chainMinQuantityTickSize := minQuantityTickSize chainMinNotional := minNotional.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6)))examples/chain/13_BroadcastMsgWithoutSimulation/example.go (2)
64-64: Consider adding context timeout for production use.While using
context.Background()is fine for examples, in production code you might want to add appropriate timeout or cancellation.-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()
Line range hint
1-100: Consider adding explanatory comments about simulation=false.While the example effectively demonstrates broadcasting without simulation, it would be helpful to add comments explaining:
- Why one might choose to disable simulation
- Potential risks and trade-offs of this approach
- Recommended use cases for this configuration
examples/chain/exchange/16_MsgLiquidatePosition/example.go (2)
62-62: Consider using context with timeoutUsing
context.Background()without a timeout could lead to hanging operations. Consider implementing a timeout for better resource management.-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()
64-66: Improve error handling for production codeSimple panic is not ideal for production code. Consider implementing proper error handling and logging.
-if err != nil { - panic(err) -} +if err != nil { + log.Printf("Failed to initialize markets assistant: %v", err) + return fmt.Errorf("markets assistant initialization failed: %w", err) +}examples/chain/exchange/13_MsgInstantBinaryOptionsMarketLaunch/example.go (2)
63-63: LGTM! Consider extracting the scale factor for better maintainability.The calculation is correct and aligns with the OracleScaleFactor. However, since both the calculation and message use the same scale factor (6), consider extracting it to a constant for better maintainability.
+const oracleScaleFactor = 6 -chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6))) +chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(oracleScaleFactor)))
Line range hint
29-35: Security: Remove hardcoded credentials from example code.The example contains hardcoded sensitive information:
- Private key: "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e"
- Password: "12345678"
These should be replaced with placeholder values or environment variables to prevent security risks if someone copies this example directly.
senderAddress, cosmosKeyring, err := chainclient.InitCosmosKeyring( os.Getenv("HOME")+"/.injectived", "injectived", "file", "inj-user", - "12345678", - "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", + os.Getenv("KEYRING_PASSWORD"), + os.Getenv("PRIVATE_KEY"), // Optional: Use keyring if not provided false, )examples/chain/exchange/5_MsgInstantExpiryFuturesMarketLaunch/example.go (1)
69-69: Consider making the token address configurableThe USDC token address is hardcoded which could make the example brittle to changes. Consider:
- Moving this to a configuration constant at the top of the file
- Adding a comment explaining the token address format
- Potentially reading it from an environment variable for flexibility
package main +const ( + // Format: factory/<creator_address>/<token_symbol> + DefaultQuoteDenom = "factory/inj17vytdwqczqz72j65saukplrktd4gyfme5agf6c/usdc" +) func main() { // ... msg := &exchangetypes.MsgInstantExpiryFuturesMarketLaunch{ // ... - QuoteDenom: "factory/inj17vytdwqczqz72j65saukplrktd4gyfme5agf6c/usdc", + QuoteDenom: os.Getenv("QUOTE_DENOM_OVERRIDE") or DefaultQuoteDenom, // ... }examples/chain/exchange/7_MsgCreateSpotMarketOrder/example.go (3)
62-66: LGTM! Consider adding context timeoutThe markets assistant initialization looks good and aligns with the refactoring objective. However, consider adding a timeout to the context for better resource management.
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()
Line range hint
28-35: Security: Remove hardcoded credentials from exampleThe example contains a hardcoded private key and password. Even in examples, it's better to use environment variables or placeholder values to prevent accidental misuse.
- "12345678", - "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", // keyring will be used if pk not provided + os.Getenv("KEYRING_PASSWORD"), + os.Getenv("PRIVATE_KEY"), // keyring will be used if pk not provided
Line range hint
69-71: Consider making market ID configurableThe hardcoded market ID makes the example less flexible. Consider making it configurable through environment variables or command-line flags.
-marketId := "0x0511ddc4e6586f3bfe1acb2dd905f8b8a82c97e1edaef654b12ca7e6031ca0fa" +marketId := os.Getenv("MARKET_ID") +if marketId == "" { + marketId = "0x0511ddc4e6586f3bfe1acb2dd905f8b8a82c97e1edaef654b12ca7e6031ca0fa" // default value +}examples/chain/exchange/6_MsgCreateSpotLimitOrder/example.go (3)
61-65: Consider adding context timeout for better resource management.While the context initialization works, adding a timeout would prevent potential resource leaks in case of long-running operations.
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()
Line range hint
27-34: Security: Avoid hardcoding sensitive information.Even in examples, it's recommended to:
- Use environment variables for private keys and passwords
- Add clear comments indicating that these are example values
- Consider using placeholder values that clearly indicate they need replacement
- "12345678", - "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", + os.Getenv("KEYRING_PASSWORD"), + os.Getenv("PRIVATE_KEY"), // Example: "5d386...1381e"
Line range hint
21-24: Standardize error handling throughout the example.The current error handling alternates between panic and return. Consider:
- Using a consistent error handling approach
- Demonstrating proper error wrapping
- Adding comments explaining the error handling strategy
Example of consistent error handling:
func main() { if err := run(); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } } func run() error { // Move the main logic here // Return errors instead of panic // Use fmt.Errorf("failed to initialize client: %w", err) for error wrapping }Also applies to: 42-47, 63-65, 91-95
examples/chain/8_OfflineSigning/example.go (1)
75-80: Consider context management improvements.While the implementation is correct, consider these improvements:
- Move context creation to the top of main() for better visibility
- Consider using context with cancellation for proper cleanup in a production environment
+func main() { + ctx := context.Background() network := common.LoadNetwork("devnet", "") // ... rest of the code ... - ctx := context.Background() marketsAssistant, err := chainclient.NewMarketsAssistant(ctx, chainClient)examples/chain/exchange/10_MsgCreateDerivativeLimitOrder/example.go (3)
62-66: Consider adding context timeout for production use.While the context initialization works for this example, in a production environment, it's recommended to use a context with timeout or cancellation to prevent potential hanging operations.
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()
Line range hint
28-34: Security: Avoid hardcoding sensitive information.The example includes hardcoded sensitive information:
- Keyring password ("12345678")
- Private key
Consider using environment variables or configuration files to demonstrate secure practices, even in examples.
- "12345678", - "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", + os.Getenv("KEYRING_PASSWORD"), + os.Getenv("PRIVATE_KEY"),
Line range hint
69-72: Document market parameters and add constants for clarity.The hard-coded values for
marketId,amount, andpricewould benefit from documentation explaining their significance and potential ranges.+// Example market parameters +const ( + // Market ID for BTC-USDT-PERP on testnet + ExampleMarketID = "0x4ca0f92fc28be0c9761326016b5a1a2177dd6375558365116b5bdda9abc229ce" + // Minimum trade amount in base currency + ExampleTradeAmount = 0.001 + // Example price in quote currency + ExamplePrice = "31000" +) + -marketId := "0x4ca0f92fc28be0c9761326016b5a1a2177dd6375558365116b5bdda9abc229ce" -amount := decimal.NewFromFloat(0.001) -price := decimal.RequireFromString("31000") //31,000 +marketId := ExampleMarketID +amount := decimal.NewFromFloat(ExampleTradeAmount) +price := decimal.RequireFromString(ExamplePrice)examples/chain/exchange/11_MsgCreateDerivativeMarketOrder/example.go (5)
63-68: Consider enhancing context management and error handlingWhile the initialization is correct, consider these improvements:
- Add a timeout to the context to prevent indefinite operations
- Include more descriptive error messages for better debugging
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel() marketsAssistant, err := chainclient.NewMarketsAssistant(ctx, chainClient) if err != nil { - panic(err) + panic(fmt.Errorf("failed to initialize markets assistant: %w", err)) }
Line range hint
28-34: Security: Remove hardcoded private key from exampleThe example contains a hardcoded private key which is a security risk. Consider using environment variables or a placeholder value with clear documentation.
- "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", // keyring will be used if pk not provided + os.Getenv("INJECTIVE_PRIVATE_KEY"), // Set your private key in INJECTIVE_PRIVATE_KEY environment variable
Line range hint
71-75: Consider making market parameters configurableThe hardcoded market ID, amount, price, and leverage values make the example less flexible. Consider making these configurable through environment variables or command-line flags.
-marketId := "0x4ca0f92fc28be0c9761326016b5a1a2177dd6375558365116b5bdda9abc229ce" -amount := decimal.NewFromFloat(0.01) -price := decimal.RequireFromString("33000") //33,000 -leverage := decimal.RequireFromString("2.5") +marketId := os.Getenv("MARKET_ID") +amount := decimal.RequireFromString(getEnvWithDefault("ORDER_AMOUNT", "0.01")) +price := decimal.RequireFromString(getEnvWithDefault("ORDER_PRICE", "33000")) +leverage := decimal.RequireFromString(getEnvWithDefault("ORDER_LEVERAGE", "2.5"))
Line range hint
116-121: Replace sleep with proper transaction confirmationUsing
time.Sleepis not a reliable way to wait for transaction processing. Consider implementing proper transaction confirmation handling.-time.Sleep(time.Second * 5) +// Wait for transaction confirmation +txResponse, err := chainClient.WaitForTx(ctx, msg.GetTxHash()) +if err != nil { + fmt.Printf("failed to confirm transaction: %v\n", err) + return +}
Line range hint
1-134: Improve error handling throughout the exampleThe example uses
panicextensively for error handling. While this might be acceptable for a simple example, consider:
- Adding proper error handling with meaningful error messages
- Implementing cleanup in case of errors
- Adding comments explaining the error scenarios
Consider wrapping the main logic in a function that returns an error and handle it in main:
func createDerivativeMarketOrder(ctx context.Context) error { // ... existing logic with error returns instead of panics ... } func main() { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() if err := createDerivativeMarketOrder(ctx); err != nil { fmt.Printf("Failed to create derivative market order: %v\n", err) os.Exit(1) } }examples/chain/authz/2_MsgExec/example.go (1)
Line range hint
29-40: Security: Avoid hardcoded private keys in examplesWhile this is an example file, it's recommended to:
- Remove hardcoded private keys
- Add comments explaining how to properly configure credentials
- Consider using environment variables or configuration files for sensitive data
Consider updating the code to use environment variables:
- "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", // keyring will be used if pk not provided + os.Getenv("GRANTER_PRIVATE_KEY"), // keyring will be used if pk not provided- "f9db9bf330e23cb7839039e944adef6e9df447b90b503d5b4464c90bea9022f3", // keyring will be used if pk not provided + os.Getenv("SENDER_PRIVATE_KEY"), // keyring will be used if pk not providedAlso, consider adding a comment block at the top of the file explaining the required environment setup:
/* Required environment variables: - GRANTER_PRIVATE_KEY: Private key for the granter account - SENDER_PRIVATE_KEY: Private key for the sender account Note: If private keys are not provided, the keyring will be used instead. */Also applies to: 41-52
examples/chain/1_LocalOrderHash/example.go (3)
62-66: Consider adding context timeout for better resource management.While the markets assistant initialization is correct, using a background context without timeout could lead to resource leaks in long-running scenarios.
Consider adding a timeout:
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel() marketsAssistant, err := chainclient.NewMarketsAssistant(ctx, chainClient)
Line range hint
28-34: Security concern: Hardcoded private key in source code.The example contains a hardcoded private key which is a security risk. Consider:
- Moving sensitive values to environment variables
- Adding a comment explaining this is a test key
- Using a clearly invalid key in the example
Replace the hardcoded private key with environment variable:
- "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", + os.Getenv("INJECTIVE_PRIVATE_KEY"),
Line range hint
82-84: Consider extracting hardcoded market IDs to constants.The market IDs are hardcoded within the order creation logic. This could make maintenance difficult if these values need to change.
Consider defining constants at the package level:
+const ( + ExampleSpotMarketID = "0x0611780ba69656949525013d947713300f56c37b6175e02f26bffa495c3208fe" + ExampleDerivativeMarketID = "0x17ef48032cb24375ba7c2e39f384e56433bcab20cbee9a7357e4cba2eb00abe6" +)Then use these constants in the order creation:
- MarketId: "0x0611780ba69656949525013d947713300f56c37b6175e02f26bffa495c3208fe", + MarketId: ExampleSpotMarketID,Also applies to: 95-97
client/keyring/keyring_config.go (2)
20-30: Consider adding backend validation.The Backend type could benefit from validation to ensure only valid backend types are used.
Consider adding a validation method:
type Backend string +func (b Backend) Validate() error { + switch b { + case BackendTest, BackendFile, BackendOS: + return nil + default: + return fmt.Errorf("invalid backend type: %s", b) + } +}
74-108: Consider consolidating duplicate logic in key configuration.
WithKeyandWithNamedKeyshare similar logic for applying key configuration options.Consider extracting the common logic:
+func applyKeyOptions(config *cosmosKeyConfig, opts ...KeyConfigOpt) error { + for optIdx, optFn := range opts { + if err := optFn(config); err != nil { + return errors.Wrapf(ErrFailedToApplyKeyConfigOption, + "key option #%d: %s", optIdx+1, err.Error()) + } + } + return nil +} func WithKey(opts ...KeyConfigOpt) ConfigOpt { return func(c *cosmosKeyringConfig) error { config := &cosmosKeyConfig{} - for optIdx, optFn := range opts { - if err := optFn(config); err != nil { - err = errors.Wrapf(ErrFailedToApplyKeyConfigOption, - "key option #%d: %s", optIdx+1, err.Error()) - return err - } + if err := applyKeyOptions(config, opts...); err != nil { + return err } c.Keys = append(c.Keys, config) return nil } }client/keyring/README.md (5)
1-5: Minor improvements to the introduction section.Consider these refinements to make the introduction more concise and clearer:
-Creates a new keyring from a variety of options. +Creates a new keyring with configurable options.🧰 Tools
🪛 LanguageTool
[style] ~3-~3: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...ring Helper Creates a new keyring from a variety of options. SeeConfigOptand related op...(A_VARIETY_OF)
25-25: Fix typo in WithNamedKey description.-* `WithNamedKey` addes a single key to the keyring, with a name. +* `WithNamedKey` adds a single key to the keyring, with a name.
34-35: Enhance security warnings for sensitive options.The security warnings for
WithPrivKeyHexandWithMnemonicshould be more prominent. Consider adding a dedicated "Security Considerations" section or using stronger warning language for these options.-* `WithPrivKeyHex` allows specifying a private key as plain-text hex. Insecure option, use for testing only. +* `WithPrivKeyHex` allows specifying a private key as plain-text hex. ⚠️ SECURITY WARNING: This is an insecure option that exposes private keys in plain text. Use only for testing in non-production environments. -* `WithMnemonic` allows specifying a mnemonic phrase as plain-text hex. Insecure option, use for testing only. +* `WithMnemonic` allows specifying a mnemonic phrase as plain-text. ⚠️ SECURITY WARNING: This is an insecure option that exposes seed phrases in plain text. Use only for testing in non-production environments.
41-82: Use obviously fake data in documentation examples.Replace the private key and mnemonic examples with clearly fake data to prevent accidental copy-paste or confusion with real credentials.
- WithPrivKeyHex("e6888cb164d52e4880e08a8a5dbe69cd62f67fde3d5906f2c5c951be553b2267"), + WithPrivKeyHex("0000000000000000000000000000000000000000000000000000000000000000"), - WithMnemonic("real simple naive ....... love"), + WithMnemonic("test word1 word2 word3 ... word24"),🧰 Tools
🪛 LanguageTool
[uncategorized] ~62-~62: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... WithKeyFrom("sender"), ), ) ``` Real world use case of keyring initialization from...(EN_COMPOUND_ADJECTIVE_INTERNAL)
86-101: Improve test documentation security and maintainability.Consider these improvements:
- Add a clear warning about not using the test passphrase in production.
- Remove the specific coverage percentage as it may become outdated.
go test -v -cover PASS -coverage: 83.1% of statements +coverage: <percentage> of statements ## Generating a Test Fixture ```bash > cd testdata > injectived keys --keyring-dir `pwd` --keyring-backend file add test-Passphrase should be
test12345678for this fixture to work.
+# For test fixtures only - DO NOT USE IN PRODUCTION
+Test passphrase:test12345678</blockquote></details> <details> <summary>examples/chain/exchange/9_MsgBatchUpdateOrders/example.go (2)</summary><blockquote> `64-69`: **Enhance context management and error handling.** While the markets initialization change looks good, consider these improvements: 1. Add a timeout to the context to prevent potential hanging 2. Implement more graceful error handling instead of panic Consider this approach: ```diff -ctx := context.Background() -marketsAssistant, err := chainclient.NewMarketsAssistant(ctx, chainClient) -if err != nil { - panic(err) -} +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel() +marketsAssistant, err := chainclient.NewMarketsAssistant(ctx, chainClient) +if err != nil { + fmt.Printf("Failed to initialize markets assistant: %v\n", err) + return +}
Line range hint
1-150: Consider restructuring the example for better maintainability.While this is an example file, consider these improvements to make it more maintainable and educational:
- Extract configuration values (market IDs, amounts, prices) to constants or a config struct
- Break down the main function into smaller, focused functions
- Add comments explaining the purpose of each step
Consider structuring the example like this:
const ( SpotMarketID = "0x0511ddc4e6586f3bfe1acb2dd905f8b8a82c97e1edaef654b12ca7e6031ca0fa" SpotAmount = 2.0 SpotPrice = 22.5 // ... other constants ) func main() { // Initialize clients chainClient := initializeChainClient() // Create and execute batch orders executeBatchOrders(chainClient) // Monitor results monitorTransaction(chainClient) } func initializeChainClient() *chainclient.ChainClient { // Client initialization logic } func executeBatchOrders(chainClient *chainclient.ChainClient) { // Order creation and execution logic } func monitorTransaction(chainClient *chainclient.ChainClient) { // Transaction monitoring logic }client/keyring/keyring_errors_test.go (6)
24-35: Consider using a more realistic invalid mnemonic.While the test correctly validates the error case, using "???" as an invalid mnemonic is not representative of real-world scenarios. Consider using a malformed BIP39 mnemonic instead (e.g., "invalid word in mnemonic phrase").
- WithMnemonic(`???`), + WithMnemonic(`invalid word in valid phrase test test`),
37-58: Enhance hex validation test cases.While the test covers basic scenarios, consider adding edge cases with almost-valid hex strings:
- Odd length hex string
- Hex string with invalid characters in the middle
- Empty string
func (s *KeyringTestSuite) TestErrHexFormatError() { requireT := s.Require() - _, _, err := NewCosmosKeyring( + testCases := []struct { + name string + hexStr string + }{ + {"empty string", ""}, + {"invalid chars", "123G45"}, + {"odd length", "123"}, + {"not hex", "nothex"}, + } + + for _, tc := range testCases { + _, _, err := NewCosmosKeyring( s.cdc, WithKey( - WithPrivKeyHex("nothex"), + WithPrivKeyHex(tc.hexStr), ), ) - ) - - requireT.ErrorIs(err, ErrHexFormatError) + requireT.ErrorIs(err, ErrHexFormatError, tc.name) + }
138-138: Address the TODO comment for multisig keys testing.The missing test coverage for multisig keys should be implemented to ensure comprehensive validation.
Would you like me to help implement the test case for multisig keys?
108-112: Consider enhancing temporary directory cleanup.While the cleanup is implemented, it could be more robust by:
- Checking the cleanup error
- Using
deferinstead oft.Cleanup- s.T().Cleanup(func() { - _ = os.RemoveAll(kbDir) - }) + defer func() { + if err := os.RemoveAll(kbDir); err != nil { + s.T().Logf("Failed to cleanup temporary directory: %v", err) + } + }()
141-155: Consider making test values more maintainable.The hardcoded values for directory path and passphrase should be moved to constants at the package level for better maintainability.
+const ( + testDataDir = "./testdata" + testPassphrase = "test12345678" + testInvalidKey = "kowabunga" +) + func (s *KeyringTestSuite) TestErrKeyRecordNotFound() { requireT := s.Require() _, _, err := NewCosmosKeyring( s.cdc, WithKeyringBackend(BackendFile), - WithKeyringDir("./testdata"), + WithKeyringDir(testDataDir), WithKey( - WithKeyFrom("kowabunga"), - WithKeyPassphrase("test12345678"), + WithKeyFrom(testInvalidKey), + WithKeyPassphrase(testPassphrase), ), )
171-193: Consider adding test case documentation.While the test is comprehensive, it would benefit from comments explaining the expected derivation path and address generation process for better maintainability.
func (s *KeyringTestSuite) TestErrUnexpectedAddress() { + // Test validates that the provided address matches the one derived from: + // 1. The private key (first test case) + // 2. The mnemonic using the default HD path (second test case) requireT := s.Require()client/keyring/keyring_test.go (1)
39-45: Consider adding TearDown method to clean up after testsWhile the setup is comprehensive, consider adding a
TearDownTestorTearDownSuitemethod to ensure proper cleanup of any global state modifications made inSetupTest.func (s *KeyringTestSuite) TearDownTest() { // Reset any global state modifications config := sdk.GetConfig() config.SetBech32PrefixForAccount("", "") // Reset to defaults config.SetCoinType(0) // Reset to default }client/chain/markets_assistant_test.go (1)
94-242: LGTM: Comprehensive test coverage for market assistant creationThe test function provides thorough coverage of:
- Multiple token types (ERC20, native, tokenFactory)
- Various market types (spot, derivative, binary options)
- Token metadata validation
- Market presence verification
Consider adding negative test cases to verify error handling:
- Invalid token metadata
- Mismatched market data
- Server errors
go.mod (1)
Line range hint
234-246: Consider documenting the replace directive strategyThe replace directives show a consistent pattern of using InjectiveLabs forks for Cosmos SDK components. This is a significant architectural decision that affects dependency management and should be documented.
Consider adding a comment block above the replace directives explaining:
- Why InjectiveLabs forks are used instead of upstream
- The versioning strategy (e.g.,
-inj-0suffix convention)- The process for updating these dependencies
Example documentation:
+# The following replace directives use InjectiveLabs forks of Cosmos SDK components +# to maintain compatibility with our custom blockchain implementation. +# Version suffix '-inj-X' indicates our fork's version.client/core/market_test.go (2)
72-101: Consider using dynamic timestamps for market expiration and settlement.The hardcoded timestamps (
1707800399and1707843599) will expire, which could make tests fail in the future. Consider using relative timestamps based on the current time to ensure tests remain valid.- ExpirationTimestamp: 1707800399, - SettlementTimestamp: 1707843599, + ExpirationTimestamp: time.Now().Add(24 * time.Hour).Unix(), + SettlementTimestamp: time.Now().Add(48 * time.Hour).Unix(),
445-454: Fix variable naming inconsistency in margin conversion tests.The variable name
humanReadablePriceis used for margin values, which is inconsistent with the method's purpose. This occurs in both regular and extended chain format tests.marginDecimals := binaryOptionMarket.QuoteToken.Decimals chainFormatMargin := expectedMargin.Mul(decimal.New(1, marginDecimals)) - humanReadablePrice := binaryOptionMarket.MarginFromChainFormat(sdkmath.LegacyMustNewDecFromStr(chainFormatMargin.String())) + humanReadableMargin := binaryOptionMarket.MarginFromChainFormat(sdkmath.LegacyMustNewDecFromStr(chainFormatMargin.String())) - assert.Assert(t, expectedMargin.Equal(humanReadablePrice)) + assert.Assert(t, expectedMargin.Equal(humanReadableMargin))Also applies to: 488-497
client/chain/chain_test_support.go (2)
37-41: Consider adding field documentationThe new fields are well-structured and follow a consistent pattern. Consider adding documentation for each field to explain their purpose in testing scenarios.
type MockChainClient struct { + // Network represents the blockchain network (e.g., mainnet, testnet) Network common.Network + // DenomsMetadataResponses contains mock responses for denom metadata queries DenomsMetadataResponses []*banktypes.QueryDenomsMetadataResponse + // QuerySpotMarketsResponses contains mock responses for spot market queries QuerySpotMarketsResponses []*exchangetypes.QuerySpotMarketsResponse + // QueryDerivativeMarketsResponses contains mock responses for derivative market queries QueryDerivativeMarketsResponses []*exchangetypes.QueryDerivativeMarketsResponse + // QueryBinaryMarketsResponses contains mock responses for binary market queries QueryBinaryMarketsResponses []*exchangetypes.QueryBinaryMarketsResponse }
370-381: Consider reducing code duplication in market query implementationsThe implementations for
FetchChainSpotMarkets,FetchChainDerivativeMarkets, andFetchChainBinaryOptionsMarketsfollow identical patterns. Consider extracting this pattern into a generic helper function to improve maintainability.+// getNextMockResponse is a generic helper for mock response queues +func getNextMockResponse[T any](responses *[]T) (T, error) { + var response T + if len(*responses) > 0 { + response = (*responses)[0] + *responses = (*responses)[1:] + return response, nil + } + return response, errors.New("there are no responses configured") +} func (c *MockChainClient) FetchChainSpotMarkets(ctx context.Context, status string, marketIDs []string) (*exchangetypes.QuerySpotMarketsResponse, error) { - var response *exchangetypes.QuerySpotMarketsResponse - var localError error - if len(c.QuerySpotMarketsResponses) > 0 { - response = c.QuerySpotMarketsResponses[0] - c.QuerySpotMarketsResponses = c.QuerySpotMarketsResponses[1:] - localError = nil - } else { - response = &exchangetypes.QuerySpotMarketsResponse{} - localError = errors.New("there are no responses configured") - } - return response, localError + return getNextMockResponse(&c.QuerySpotMarketsResponses) }Also applies to: 449-460, 564-575
client/chain/chain.go (1)
296-296: Add documentation for the GetNetwork method.The method looks good, but consider adding documentation to describe its purpose and return value.
Add a comment like:
+// GetNetwork returns the network configuration associated with this chain client GetNetwork() common.Networkexamples/exchange/oracle/1_StreamPrices/example.go (1)
70-73: Questioning the Necessity of Re-initializingexchangeClientAfter initializing
chainClientandmarketsAssistant, you re-initializeexchangeClient. Since you've shifted to usingchainClientfor operations, consider whether re-instantiatingexchangeClientis necessary.If
exchangeClientis only utilized for theStreamPricesmethod, check if there's an equivalent method available inchainClient. This would promote consistency and reduce dependencies.client/chain/markets_assistant_test_support.go (1)
228-251: Nitpick: Consistent Formatting and Ordering of FieldsFor better readability and maintainability, consider ordering the struct fields consistently across all market creation functions. This makes it easier to compare and spot discrepancies between market configurations.
client/core/market.go (4)
194-194: UseLegacyMustNewDecFromStrinQuantityToChainFormatInstead of ignoring the error from
LegacyNewDecFromStr, useLegacyMustNewDecFromStrto simplify the code and ensure safety.Apply this diff:
- valueInChainFormat, _ := sdkmath.LegacyNewDecFromStr(quantizedValue.String()) + valueInChainFormat := sdkmath.LegacyMustNewDecFromStr(quantizedValue.String())
202-202: UseLegacyMustNewDecFromStrinPriceToChainFormatSimilarly, replace
LegacyNewDecFromStrwithLegacyMustNewDecFromStrto handle errors appropriately.Apply this diff:
- valueInChainFormat, _ := sdkmath.LegacyNewDecFromStr(quantizedValue.String()) + valueInChainFormat := sdkmath.LegacyMustNewDecFromStr(quantizedValue.String())
221-221: UseLegacyMustNewDecFromStrinCalculateMarginInChainFormatUpdating to
LegacyMustNewDecFromStrimproves code clarity and error handling.Apply this diff:
- valueInChainFormat, _ := sdkmath.LegacyNewDecFromStr(quantizedMargin.String()) + valueInChainFormat := sdkmath.LegacyMustNewDecFromStr(quantizedMargin.String())
229-229: UseLegacyMustNewDecFromStrinNotionalToChainFormatEnsure consistent error handling by using
LegacyMustNewDecFromStr.Apply this diff:
- valueInChainFormat, _ := sdkmath.LegacyNewDecFromStr(chainFormattedValue.String()) + valueInChainFormat := sdkmath.LegacyMustNewDecFromStr(chainFormattedValue.String())client/keyring/keyring.go (5)
144-201: Consider refactoring common logic to reduce code duplicationThe functions
fromPrivkeyHex(lines 144-201) andfromMnemonic(lines 202-267) contain similar logic for handling key names, validating addresses, checking private key matches, and adding keys to the keyring. Refactoring shared code into helper functions would improve maintainability and reduce duplication.Also applies to: 202-267
398-440: Extend support for additional key types or provide clearer messagingThe
checkKeyRecordfunction currently does not supportTypeOfflineandTypeMultikey types, returning errors that they are "not supported yet." If support for these key types is planned, consider implementing handling for them. If not, ensure that the error messages clearly communicate the lack of support to the user.
490-496: Avoid hardcoding magic numbersThe size parameter in
randPhrase(size int)is currently hardcoded to64in the calling function. Consider defining a constant or providing documentation to explain why 64 bytes are used for the passphrase length to improve code readability.Example:
const passphraseLength = 64 // ... tmpPhrase, err := randPhrase(passphraseLength)
144-267: Ensure consistent error handling and wrappingThroughout
fromPrivkeyHexandfromMnemonic, errors are wrapped with context usingerrors.Wrapf. Ensure that all errors, including those returned directly (return emptyCosmosAddress, err), are consistently wrapped to provide clear error messages.
158-191: Simplify key name assignment logicThe logic for assigning
keyNamecan be simplified to improve readability. Currently, there are multiple checks and assignments based on conditions. Consider consolidating these to make the code more straightforward.Example:
if keyConfig.KeyFrom != "" && keyConfig.Name == "" { keyName = keyConfig.KeyFrom } else if keyConfig.Name == "" { keyName = defaultKeyringKeyName } else { keyName = keyConfig.Name }client/chain/markets_assistant.go (4)
Line range hint
242-248: Fix pagination logic ininitializeTokensFromChainDenomsThe pagination logic does not update
nextKey, resulting in only one page being fetched. To correctly paginate and fetch all tokens, ensurenextKeyis updated with the result's pagination next key.Apply the following fix:
func (assistant *MarketsAssistant) initializeTokensFromChainDenoms(ctx context.Context, chainClient ChainClient) { var denomsMetadata []banktypes.Metadata var nextKey []byte for readNextPage := true; readNextPage; readNextPage = len(nextKey) > 0 { pagination := query.PageRequest{Key: nextKey} result, err := chainClient.GetDenomsMetadata(ctx, &pagination) if err != nil { panic(err) } denomsMetadata = append(denomsMetadata, result.GetMetadatas()...) + nextKey = result.Pagination.NextKey } // Rest of the code }
Line range hint
245-247: Avoid panicking on error; return error insteadIn
initializeTokensFromChainDenoms, panicking on error is discouraged. Consider returning the error to allow the caller to handle it appropriately.Update the function to return an error:
-func (assistant *MarketsAssistant) initializeTokensFromChainDenoms(ctx context.Context, chainClient ChainClient) { +func (assistant *MarketsAssistant) initializeTokensFromChainDenoms(ctx context.Context, chainClient ChainClient) error { // Existing code if err != nil { - panic(err) + return err } // Existing codeUpdate the caller to handle the error:
func NewMarketsAssistantWithAllTokens(ctx context.Context, exchangeClient exchange.ExchangeClient, chainClient ChainClient) (MarketsAssistant, error) { assistant := newMarketsAssistant() - assistant.initializeTokensFromChainDenoms(ctx, chainClient) + if err := assistant.initializeTokensFromChainDenoms(ctx, chainClient); err != nil { + return assistant, err + } err := assistant.initializeFromChain(ctx, chainClient) return assistant, err }
Line range hint
263-269: Ensuredecimalsis non-negativeIf
denomMetadata.GetDenomUnits()is empty,decimalsremains-1. This negative value may cause issues in calculations. Consider settingdecimalsto0if no denom units are found.Apply this fix:
var decimals int32 = -1 for _, denomUnit := range denomMetadata.GetDenomUnits() { exponent := int32(denomUnit.GetExponent()) if exponent > decimals { decimals = exponent } } +if decimals == -1 { + decimals = 0 +}
31-31: Add documentation forbinaryOptionMarketsfieldConsider adding a comment to document the
binaryOptionMarketsfield in theMarketsAssistantstruct for consistency and clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (53)
.github/workflows/run-tests.yml(1 hunks).pre-commit-config.yaml(1 hunks)auth_vote/authz_vote.go(0 hunks)client/cert/embed.go(0 hunks)client/cert/mainnet.crt(0 hunks)client/cert/testnet.crt(0 hunks)client/chain/chain.go(2 hunks)client/chain/chain_test_support.go(6 hunks)client/chain/markets_assistant.go(6 hunks)client/chain/markets_assistant_test.go(3 hunks)client/chain/markets_assistant_test_support.go(2 hunks)client/common/embeded.go(0 hunks)client/common/util.go(0 hunks)client/core/market.go(1 hunks)client/core/market_test.go(2 hunks)client/keyring/README.md(1 hunks)client/keyring/errors.go(1 hunks)client/keyring/key_config.go(1 hunks)client/keyring/keyring.go(1 hunks)client/keyring/keyring_config.go(1 hunks)client/keyring/keyring_errors_test.go(1 hunks)client/keyring/keyring_test.go(1 hunks)client/keyring/testdata/keyring-file/263117aad9ebf5759a8272ad2ae4968dd12d4602.address(1 hunks)client/keyring/testdata/keyring-file/310322fcb0937ade77a9ba0d128f9e7f17312796.address(1 hunks)client/keyring/testdata/keyring-file/keyhash(1 hunks)client/keyring/testdata/keyring-file/test.info(1 hunks)client/keyring/testdata/keyring-file/test2.info(1 hunks)client/metadata/assets/devnet-1.ini(0 hunks)client/metadata/assets/devnet.ini(0 hunks)client/metadata/assets/embed.go(0 hunks)client/metadata/fetch_metadata.go(0 hunks)client/tm/tmclient.go(2 hunks)examples/chain/13_BroadcastMsgWithoutSimulation/example.go(2 hunks)examples/chain/1_LocalOrderHash/example.go(2 hunks)examples/chain/4_MsgBatchCreateSpotLimitOrders/example.go(2 hunks)examples/chain/5_MsgBatchCreateDerivativeLimitOrders/example.go(2 hunks)examples/chain/8_OfflineSigning/example.go(2 hunks)examples/chain/authz/2_MsgExec/example.go(2 hunks)examples/chain/exchange/10_MsgCreateDerivativeLimitOrder/example.go(2 hunks)examples/chain/exchange/11_MsgCreateDerivativeMarketOrder/example.go(2 hunks)examples/chain/exchange/13_MsgInstantBinaryOptionsMarketLaunch/example.go(3 hunks)examples/chain/exchange/16_MsgLiquidatePosition/example.go(2 hunks)examples/chain/exchange/24_MsgUpdateSpotMarket/example.go(2 hunks)examples/chain/exchange/25_MsgUpdateDerivativeMarket/example.go(2 hunks)examples/chain/exchange/3_MsgInstantSpotMarketLaunch/example.go(2 hunks)examples/chain/exchange/4_MsgInstantPerpetualMarketLaunch/example.go(2 hunks)examples/chain/exchange/5_MsgInstantExpiryFuturesMarketLaunch/example.go(2 hunks)examples/chain/exchange/6_MsgCreateSpotLimitOrder/example.go(2 hunks)examples/chain/exchange/7_MsgCreateSpotMarketOrder/example.go(2 hunks)examples/chain/exchange/9_MsgBatchUpdateOrders/example.go(2 hunks)examples/exchange/oracle/1_StreamPrices/example.go(1 hunks)examples/exchange/oracle/2_Price/example.go(1 hunks)go.mod(1 hunks)
💤 Files with no reviewable changes (10)
- auth_vote/authz_vote.go
- client/cert/embed.go
- client/cert/mainnet.crt
- client/cert/testnet.crt
- client/common/embeded.go
- client/common/util.go
- client/metadata/assets/devnet-1.ini
- client/metadata/assets/devnet.ini
- client/metadata/assets/embed.go
- client/metadata/fetch_metadata.go
✅ Files skipped from review due to trivial changes (5)
- client/keyring/testdata/keyring-file/263117aad9ebf5759a8272ad2ae4968dd12d4602.address
- client/keyring/testdata/keyring-file/310322fcb0937ade77a9ba0d128f9e7f17312796.address
- client/keyring/testdata/keyring-file/keyhash
- client/keyring/testdata/keyring-file/test.info
- client/keyring/testdata/keyring-file/test2.info
🧰 Additional context used
🪛 LanguageTool
client/keyring/README.md
[style] ~3-~3: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...ring Helper Creates a new keyring from a variety of options. See ConfigOpt and related op...
(A_VARIETY_OF)
[grammar] ~24-~24: Did you mean “having to alias” or “having aliased”?
Context: ...ds a single key to the keyring, without having alias name. * WithNamedKey addes a single k...
(WITHOUT_VBG_VB)
[uncategorized] ~62-~62: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... WithKeyFrom("sender"), ), ) ``` Real world use case of keyring initialization from...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 Gitleaks
client/keyring/keyring_test.go
285-285: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
286-286: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (58)
.pre-commit-config.yaml (1)
5-6: Verify the necessity of excluding keyring testdata
The addition of client/keyring/testdata/.* to exclusions appears related to the new keyring helper functionality. However, we should verify if all files in this directory truly need to be excluded from pre-commit checks.
✅ Verification successful
Exclusion of keyring testdata files is justified
The files in client/keyring/testdata contain sensitive cryptographic material:
.addressand.infofiles contain encrypted JWE (JSON Web Encryption) payloadskeyhashfile contains bcrypt password hashes- All files contain security-sensitive test fixtures that should be excluded from pre-commit hooks to prevent unintended modifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the contents of keyring testdata directory to understand what types of files are being excluded
# List all files in the keyring testdata directory
fd . "client/keyring/testdata" --type f
# Check for potential sensitive files that should be excluded
rg -l "PRIVATE|SECRET|PASSWORD|KEY" "client/keyring/testdata"
Length of output: 413
Script:
#!/bin/bash
# Let's check the content and format of these files to understand if they contain sensitive data
# Check the content of .address files
head -n 5 client/keyring/testdata/keyring-file/*.address
# Check the content of .info files
head -n 5 client/keyring/testdata/keyring-file/*.info
# Check the content of keyhash file
head -n 5 client/keyring/testdata/keyring-file/keyhash
# Check file types to see if they are binary or text
file client/keyring/testdata/keyring-file/*
Length of output: 3729
client/keyring/errors.go (1)
1-20: LGTM! Well-structured error definitions.
The error variables are well-organized, follow Go conventions, and provide a comprehensive set of error cases for keyring operations. The use of github.com/pkg/errors is appropriate for error handling.
client/keyring/key_config.go (1)
16-18: LGTM! Well-structured functional options pattern.
The KeyConfigOpt type is well-defined and follows the functional options pattern, providing a clean and extensible configuration API.
examples/exchange/oracle/2_Price/example.go (2)
9-12: LGTM! Import changes align with the architectural shift.
The new imports support the transition from exchangeclient to chainclient architecture.
68-71: 🛠️ Refactor suggestion
Review the need for exchange client initialization.
Given the architectural shift towards using chainClient, verify if the exchangeClient is still necessary. If it is:
- Improve error handling
- Document why both clients are needed
- exchangeClient, err := exchangeclient.NewExchangeClient(network)
- if err != nil {
- panic(err)
- }
+ exchangeClient, err := exchangeclient.NewExchangeClient(network)
+ if err != nil {
+ return fmt.Errorf("failed to create exchange client: %w", err)
+ }
+
+ // TODO: Document why both chain and exchange clients are needed in this example✅ Verification successful
Based on the search results, I can now provide a final response. The results show that:
- The
exchangeClientis used extensively throughout the examples for interacting with exchange-related functionality - The
chainClientandexchangeClientserve different purposes:exchangeClient: Handles exchange-specific operations like orderbooks, markets, streaming updateschainClient: Handles blockchain operations like transactions, queries, and IBC
Both exchange and chain clients are necessary in this example.
The exchange client is required for oracle price functionality as shown by its usage in other oracle examples (oracle/1_StreamPrices, oracle/2_Price, oracle/3_OracleList). However, the error handling should be improved as suggested.
- exchangeClient, err := exchangeclient.NewExchangeClient(network)
- if err != nil {
- panic(err)
- }
+ exchangeClient, err := exchangeclient.NewExchangeClient(network)
+ if err != nil {
+ return fmt.Errorf("failed to create exchange client: %w", err)
+ }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other examples using both chain and exchange clients
rg -l "NewChainClient.*NewExchangeClient" examples/
Length of output: 53
Script:
#!/bin/bash
# Let's try a more comprehensive search to understand the usage patterns
# 1. Find all files using exchangeClient
echo "Files using NewExchangeClient:"
rg "NewExchangeClient" examples/ -A 5
echo -e "\nFiles using chainClient:"
rg "NewChainClient" examples/ -A 5
echo -e "\nCheck the exchange client package to understand its purpose:"
rg -A 10 "type ExchangeClient"
Length of output: 128961
examples/chain/exchange/25_MsgUpdateDerivativeMarket/example.go (2)
9-9: LGTM! Import changes align with the refactoring objectives.
The replacement of exchangeclient with direct rpchttp client usage simplifies the dependency chain.
63-65: Verify consistent refactoring across example files.
Let's ensure this refactoring pattern is consistently applied across other market-related examples.
✅ Verification successful
Let me check the actual implementation in these files to verify consistency.
Decimal conversion patterns are consistent across market examples
The verification shows that the decimal conversion patterns are consistently implemented across all market-related examples:
- Spot markets (3, 24): Use 6 decimals for price and notional, 18 decimals for quantity
- Derivative markets (4, 5, 13, 25): Use 6 decimals for price and notional, no conversion for quantity
The changes in file 25 are aligned with the established patterns in similar market types, maintaining consistency in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other example files have been similarly updated to remove exchangeclient usage
# Check for any remaining exchangeclient imports in examples
echo "Checking for remaining exchangeclient imports in examples:"
rg -l "exchangeclient" examples/chain/exchange/
# Check for similar decimal calculations in other market examples
echo "Checking for similar market calculations in other examples:"
rg -l "LegacyNewDecFromIntWithPrec.*6" examples/chain/exchange/
Length of output: 771
Script:
#!/bin/bash
# Check the actual implementation of decimal calculations in market-related files
for file in examples/chain/exchange/{3,4,5,13,24,25}_*/example.go; do
echo "=== $file ==="
rg "LegacyNewDecFromIntWithPrec.*6" "$file" -B 1 -A 1
done
Length of output: 3643
examples/chain/exchange/24_MsgUpdateSpotMarket/example.go (2)
9-14: LGTM! Import changes align with the refactoring objectives.
The transition from exchangeclient to chainclient and the reorganization of imports reflect the broader architectural improvements mentioned in the PR objectives.
63-67: 🛠️ Refactor suggestion
Document hardcoded decimal values and consider making them configurable.
The current implementation uses hardcoded decimal values (6 and 18) which appear to be specific to USDC and INJ tokens respectively. This makes the example less reusable for other token pairs and could lead to incorrect calculations if used as a reference for different markets.
Consider:
- Adding comments explaining these specific decimal values
- Making these values configurable parameters
- Adding a warning that these calculations are specific to INJ/USDC pair
Let's verify if these decimal values are consistent with other market examples:
Here's a suggested improvement:
+ // Decimal values specific to INJ/USDC pair
+ const (
+ usdcDecimals = 6 // USDC has 6 decimal places
+ injDecimals = 18 // INJ has 18 decimal places
+ )
- chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6)))
- chainMinPriceTickSize = chainMinPriceTickSize.Quo(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(18)))
+ chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(usdcDecimals)))
+ chainMinPriceTickSize = chainMinPriceTickSize.Quo(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(injDecimals)))
- chainMinQuantityTickSize := minQuantityTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(18)))
- chainMinNotional := minNotional.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6)))
+ chainMinQuantityTickSize := minQuantityTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(injDecimals)))
+ chainMinNotional := minNotional.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(usdcDecimals)))examples/chain/exchange/3_MsgInstantSpotMarketLaunch/example.go (2)
9-9: LGTM! Import changes align with the architectural shift
The addition of the rpchttp import and removal of exchangeclient import align with the broader changes to streamline client interactions.
63-67: Verify decimal calculations and add documentation
The decimal calculations use hardcoded values that might not be appropriate for all token pairs:
- chainMinPriceTickSize effectively divides by 10^12 (multiplies by 10^6 then divides by 10^18)
- chainMinQuantityTickSize multiplies by 10^18
- chainMinNotional multiplies by 10^6
Please verify these calculations are correct for INJ/USDC pair and consider:
- Adding comments explaining the reasoning behind these specific decimal places
- Making these values configurable if they need to vary by token pair
examples/chain/13_BroadcastMsgWithoutSimulation/example.go (2)
8-8: LGTM! Import changes align with the refactoring.
The reorganized imports properly support the new client initialization approach while maintaining Go conventions.
Also applies to: 13-13, 15-15
64-69: LGTM! Markets assistant initialization refactored successfully.
The new initialization approach using chainclient.NewMarketsAssistant with direct client instance is cleaner and more straightforward.
examples/chain/exchange/16_MsgLiquidatePosition/example.go (2)
9-9: LGTM! Import changes align with the refactoring objectives
The addition of rpchttp and common imports supports the transition from exchangeclient to chainclient while maintaining necessary functionality.
Also applies to: 16-16
69-69: Verify market ID format consistency
The market ID format used here (0x17ef...) should be consistent across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information to verify the market ID format consistency across the codebase.
Market ID format is consistent across the codebase
The market IDs consistently follow the format of "0x" followed by 64 hexadecimal characters (32 bytes) across all examples and implementations. This is evidenced by all market ID usages in:
- Exchange examples (spot, derivatives)
- Chain message examples
- Query implementations
- Event definitions
- Proto definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check market ID format consistency across the codebase
# Expected format: 0x followed by 64 hex characters
# Search for market ID declarations
rg -U 'marketId.*:.*"0x[a-fA-F0-9]+"' --type go
# Check for any different market ID formats that might be inconsistent
rg -U 'marketId.*:.*"[^"]*"' --type go
Length of output: 370573
examples/chain/exchange/4_MsgInstantPerpetualMarketLaunch/example.go (2)
9-9: LGTM: Import changes align with client refactoring
The addition of rpchttp import and removal of the context package align with the broader refactoring effort to streamline the client architecture.
69-69: Document factory denom pattern and verify token address
The hardcoded factory denom pattern factory/inj17vytdwqczqz72j65saukplrktd4gyfme5agf6c/usdc should be documented to explain:
- When to use factory pattern vs native denoms
- How to derive the correct factory address for different tokens
Let's verify if this factory address is consistently used across the codebase:
examples/chain/exchange/5_MsgInstantExpiryFuturesMarketLaunch/example.go (2)
9-9: LGTM: Import addition aligns with architecture changes
The addition of the cometbft rpchttp client import is consistent with the transition to the new client architecture.
63-63: Verify the hardcoded precision value
The calculation uses a fixed precision of 6 for chainMinPriceTickSize. Please verify if this value is appropriate for all possible quote tokens.
✅ Verification successful
Let me gather more information about the quote token handling to make a more informed decision.
Based on the gathered information, I can now provide a final response:
The hardcoded precision value of 6 is correct and consistent
The precision value of 6 is consistently used across all market types (spot, perpetual, futures, binary options) for price tick size calculations. This is a standardized approach where:
- For derivative markets (futures, perpetual, binary options): A simple multiplication with 10^6 precision is applied
- For spot markets: The same 10^6 precision is applied, with an additional adjustment using 10^18 to account for token decimals
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of price tick size calculations to verify consistency
rg -A 2 "MinPriceTickSize.*Prec.*6"
# Look for any configuration or constant definitions related to price tick size precision
ast-grep --pattern 'const $_PricePrecision = 6'
Length of output: 2822
Script:
#!/bin/bash
# Search for quote token related configurations and market initialization
rg -A 5 "quoteToken.*"
# Look for any market configuration or settings related to decimals/precision
ast-grep --pattern 'type Market struct {
$$$
}'
# Search for any decimal or precision related constants
rg -A 2 "Dec.*Prec|DECIMAL.*PREC"
Length of output: 17650
examples/chain/exchange/7_MsgCreateSpotMarketOrder/example.go (1)
9-9: Verify the impact of migration from tendermint to cometbft
The switch from tendermint to cometbft RPC client is a significant change. Let's ensure this migration is consistent across the codebase.
Also applies to: 14-16
✅ Verification successful
Based on the comprehensive search results, I can now generate the final response:
Migration from tendermint to cometbft is consistent across the codebase
The codebase shows a complete and consistent migration from tendermint to cometbft RPC client. All imports of rpchttp are now using the cometbft package (github.com/cometbft/cometbft/rpc/client/http), and there are no remaining tendermint RPC client imports. The only remaining tendermint references are for crypto and hash utilities which are part of the expected dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining tendermint imports that need migration
rg -l 'github.com/tendermint' --type go
# Check for mixed usage of tendermint and cometbft
rg -l 'github.com/cometbft|github.com/tendermint' --type go | xargs rg -l 'tendermint'
Length of output: 517
Script:
#!/bin/bash
# Let's examine the actual usage context in these files to determine if they are imports or just string references
rg 'github.com/tendermint' -A 2 --type go
# Also check cometbft usage to understand the migration pattern
rg 'github.com/cometbft' -A 2 --type go
Length of output: 51854
examples/chain/exchange/6_MsgCreateSpotLimitOrder/example.go (2)
9-9: LGTM! Import changes align with the client transition.
The imports correctly reflect the transition from exchangeclient to chainclient and include necessary dependencies.
Also applies to: 14-14, 16-16
Line range hint 1-124: Verify complete removal of exchangeclient references.
The changes align with the PR objectives of refactoring markets tokens initialization. Let's verify all exchangeclient references have been removed.
✅ Verification successful
No exchangeclient references found in the reviewed file.
The file under review examples/chain/exchange/6_MsgCreateSpotLimitOrder/example.go does not contain any references to exchangeclient. The code correctly uses chainclient and marketsAssistant for market token initialization, which aligns with the PR objectives.
While there are many exchangeclient references in other example files, they are not relevant to this specific review as they demonstrate different use cases and are not part of the refactoring scope of this PR.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining exchangeclient references in the codebase
rg -i "exchangeclient" --type go
Length of output: 57020
examples/chain/4_MsgBatchCreateSpotLimitOrders/example.go (2)
9-9: LGTM! Import changes are appropriate.
The addition of the rpchttp import is necessary for the HTTP RPC client functionality, and the import organization follows Go conventions.
Also applies to: 16-16
64-69: LGTM! Verify similar patterns in other files.
The markets assistant initialization has been properly updated to use the chain client instead of the exchange client, with appropriate error handling in place.
Let's verify that similar patterns have been updated consistently across other example files:
✅ Verification successful
All chain examples consistently use the new markets assistant initialization pattern
The verification shows that:
- All chain-related examples (
examples/chain/*) consistently use the newchainclient.NewMarketsAssistantpattern - Exchange-related examples (
examples/exchange/*) correctly maintain their usage ofexchangeclient.NewExchangeClient - The only exceptions are two oracle examples that use both patterns, which is expected as they interact with both chain and exchange
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of old exchange client initialization patterns
# and verify consistent usage of the new markets assistant initialization.
# Check for any remaining exchangeclient.NewExchangeClient usage
echo "Checking for remaining exchangeclient.NewExchangeClient usage:"
rg "exchangeclient\.NewExchangeClient"
# Check for consistent usage of new markets assistant initialization
echo -e "\nVerifying consistent markets assistant initialization pattern:"
rg "NewMarketsAssistant"
# Look for potential missed updates in other example files
echo -e "\nChecking other example files for potential updates needed:"
fd -t f example.go examples/chain/ --exec rg -l "marketsAssistant"
Length of output: 11561
examples/chain/8_OfflineSigning/example.go (2)
10-10: LGTM! Import changes align with the new client architecture.
The import modifications correctly reflect the transition from exchangeclient to chainclient for market operations.
Also applies to: 14-16
76-76: Verify the impact of NewMarketsAssistant API change.
The change from NewMarketsAssistantInitializedFromChain to NewMarketsAssistant might affect other examples and implementations.
✅ Verification successful
API change is properly implemented across the codebase
The change from NewMarketsAssistantInitializedFromChain to NewMarketsAssistant has been consistently implemented across all examples. The old API is marked as deprecated in the source code, and all example implementations are using the new NewMarketsAssistant function with the correct signature (ctx and chainClient parameters).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old API usage and verify consistent implementation across examples
# Check for old API usage
rg "NewMarketsAssistantInitializedFromChain" --type go
# Check implementation consistency in other examples
fd -e go -p "example.go$" -x rg -l "NewMarketsAssistant"
Length of output: 1206
Script:
#!/bin/bash
# Let's check the actual implementation in the files to verify the API change impact
# First, check the implementation in markets_assistant.go to understand the change
rg -A 5 "NewMarketsAssistant" client/chain/markets_assistant.go
# Then check a few example implementations to verify consistency
echo "=== Example implementations ==="
for file in examples/chain/8_OfflineSigning/example.go examples/chain/exchange/6_MsgCreateSpotLimitOrder/example.go examples/chain/exchange/10_MsgCreateDerivativeLimitOrder/example.go; do
echo "=== $file ==="
rg -A 2 "NewMarketsAssistant" "$file"
done
Length of output: 2605
examples/chain/exchange/10_MsgCreateDerivativeLimitOrder/example.go (1)
9-16: LGTM! Import changes align with the new implementation.
The updated imports correctly reflect the transition from exchangeclient to chainclient for market operations.
examples/chain/exchange/11_MsgCreateDerivativeMarketOrder/example.go (1)
9-9: LGTM! Import changes align with the refactoring
The new imports support the transition from exchangeclient to chainclient and are correctly placed.
Also applies to: 16-16
client/tm/tmclient.go (1)
21-21: LGTM: Interface addition follows established patterns
The new GetStatus method is a well-structured addition to the TendermintClient interface, maintaining consistency with existing methods in terms of parameter and return types.
examples/chain/authz/2_MsgExec/example.go (2)
13-18: LGTM! Import statements are well-organized
The new imports properly support the refactored client architecture while maintaining good organization (standard library, external, and internal imports).
80-85: LGTM! Markets assistant initialization follows the new pattern
The change aligns with the broader refactoring effort to streamline client interactions. The error handling and context usage are properly maintained.
Let's verify this pattern is consistently applied across other example files:
✅ Verification successful
Markets assistant initialization is consistently applied across the codebase
The verification shows that:
- No instances of the old
NewMarketsAssistantInitializedFromChainpattern were found - The new
NewMarketsAssistant(ctx, chainClient)pattern is consistently used across all relevant example files in both chain and exchange directories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of NewMarketsAssistant across example files
# Expected: All examples should use NewMarketsAssistant with chainClient
# Search for any remaining usage of NewMarketsAssistantInitializedFromChain
echo "Checking for old initialization pattern..."
rg "NewMarketsAssistantInitializedFromChain" examples/
# Search for new initialization pattern
echo "Checking for new initialization pattern..."
rg "NewMarketsAssistant.*chainClient" examples/
Length of output: 2004
examples/chain/1_LocalOrderHash/example.go (1)
9-11: LGTM! Import changes align with the new architecture.
The new imports support the transition to chainclient and provide proper decimal handling for order values.
client/keyring/keyring_config.go (4)
1-5: LGTM! Clean package structure and imports.
The package name appropriately reflects its purpose and imports are minimal.
7-18: LGTM! Well-structured configuration using the functional options pattern.
The design allows for flexible and maintainable configuration with clear separation of concerns.
32-41: Add path validation for keyring directory.
The keyring directory path should be validated to prevent directory traversal attacks.
[security]
Consider adding path validation:
func WithKeyringDir(v string) ConfigOpt {
return func(c *cosmosKeyringConfig) error {
if v != "" {
+ // Validate path is clean and absolute
+ if !filepath.IsAbs(v) {
+ return errors.New("keyring directory must be an absolute path")
+ }
+ if filepath.Clean(v) != v {
+ return errors.New("keyring directory contains invalid path elements")
+ }
c.KeyringDir = v
}
return nil
}
}110-120: Verify default key existence.
WithDefaultKey should verify that the specified key exists in the configuration.
Let's verify the default key validation:
examples/chain/exchange/9_MsgBatchUpdateOrders/example.go (1)
9-16: LGTM! Import changes align with the client refactoring.
The imports correctly reflect the transition from exchangeclient to chainclient as part of the broader refactoring effort.
client/keyring/keyring_errors_test.go (4)
10-22: LGTM! Well-structured error test case.
The test effectively validates the error handling for an invalid keyring backend.
60-82: LGTM! Comprehensive incompatibility testing.
The test effectively validates incompatible configuration combinations for both mnemonic and private key scenarios with ledger usage.
84-90: LGTM! Simple and focused test case.
The test effectively validates the error when no key details are provided.
157-169: LGTM! Clear conflict validation.
The test effectively validates the error when conflicting private key and mnemonic are provided.
client/keyring/keyring_test.go (2)
1-19: LGTM: Well-organized package structure and imports
The imports are properly organized and grouped logically, following Go best practices.
47-214: Verify test coverage for error scenarios
While the happy path scenarios are well tested, consider adding test cases for:
- Invalid private keys
- Malformed mnemonics
- Network errors during keyring operations
- Concurrent access to the keyring
client/chain/markets_assistant_test.go (2)
5-5: LGTM: Import changes are well-organized and necessary
The new imports support JSON handling, testing assertions, and type definitions needed for the enhanced test coverage.
Also applies to: 11-17
259-261: LGTM: Mock responses properly initialized
The changes correctly integrate the MockChainClient with empty responses for all market types, maintaining consistency with the new architecture.
go.mod (2)
Line range hint 3-3: Verify Go 1.22 compatibility across dependencies
The module uses Go 1.22, which is relatively new. Some dependencies might not be fully tested with this version.
Let's check for potential compatibility issues:
#!/bin/bash
# Description: Check for Go version constraints in dependencies
# Look for go.mod files in dependencies that might specify different Go versions
echo "Go version requirements in dependencies:"
fd -t f 'go.mod' -x rg -l '^go 1\.' {}21-21: Verify the necessity of direct go-bip39 dependency
The github.com/cosmos/go-bip39 package is now listed as a direct dependency, but it's commonly included transitively through cosmos-sdk. This might indicate duplicate BIP39 implementations in the codebase.
Let's verify the usage and necessity of this direct dependency:
✅ Verification successful
Direct go-bip39 dependency is necessary
The direct dependency on github.com/cosmos/go-bip39 is justified as it's being used directly in the codebase:
client/keyring/key_config.go: Usesbip39.IsMnemonicValid()for mnemonic validationchain/crypto/hd/algorithm.go: Usesbip39.NewSeedWithErrorChecking()for seed generation
While cosmos-sdk is also a dependency, having direct control over the BIP39 implementation version is important for cryptographic operations and key management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if go-bip39 is directly used in the codebase or if it's only used through cosmos-sdk
# Search for direct imports of go-bip39
echo "Direct imports of go-bip39:"
rg -l 'github.com/cosmos/go-bip39'
# Search for BIP39-related functionality to check for potential duplication
echo "\nBIP39-related code:"
rg -l 'GenerateMnemonic|MnemonicToSeed'
Length of output: 377
Script:
#!/bin/bash
# Let's try a different approach to verify the dependency usage
# Check if go-bip39 is a transitive dependency through cosmos-sdk
echo "Checking go.mod for cosmos-sdk version:"
grep "github.com/cosmos/cosmos-sdk" go.mod
# Check for any direct usage of bip39 related code
echo -e "\nChecking for BIP39 imports and usage:"
rg -l "bip39"
# Look for specific BIP39 function calls with context
echo -e "\nBIP39 function calls with context:"
rg "bip39\." -A 2 -B 2
Length of output: 1125
client/core/market_test.go (1)
360-465: LGTM! Comprehensive test coverage for binary option market conversions.
The tests thoroughly cover all conversion scenarios and maintain consistency with the existing test patterns for spot and derivative markets.
client/chain/chain_test_support.go (2)
Line range hint 12-31: LGTM: Import statements are well-organized
The new imports are properly organized and necessary for the added functionality.
774-776: LGTM: Clean getter implementation
The GetNetwork method is a straightforward and correct implementation of a getter for the Network field.
client/chain/chain.go (1)
2651-2653: LGTM: Implementation is clean and correct.
The GetNetwork implementation properly returns the network field from the chainClient struct.
examples/exchange/oracle/1_StreamPrices/example.go (1)
70-73: Verify the Existence of Equivalent Streaming Methods in chainClient
Ensure that chainClient does not already provide a streaming method for prices, which could replace the need for exchangeClient.
Run the following script to search for streaming methods in chainClient:
client/chain/markets_assistant_test_support.go (2)
210-210: Verify: Correct OracleType Assignment
The OracleType is set to oracletypes.OracleType_Band for the BTC/USDT derivative market. Confirm that this is the intended oracle type. If a different oracle provider is to be used (e.g., Chainlink, Binance), update accordingly.
168-251: Verify: Market IDs and Denoms
Ensure that the MarketId, BaseDenom, and QuoteDenom values are correct and correspond to the actual markets and tokens on the chain. Mismatches can lead to issues when interacting with the markets.
Run the following script to cross-reference the market IDs and denominations with existing records:
Please confirm that there are no duplicates and that all IDs and denominations are accurate.
✅ Verification successful
Let me analyze the market IDs and denoms in more detail with another verification script.
This will help us verify if these market definitions are consistent within test files and if there are any duplicates or inconsistencies in the complete market definitions.
Let me try one final verification with a modified pattern to better match the Go syntax.
Based on the comprehensive verification results, I can now provide the final response:
Market IDs and Denoms are Correctly Defined ✓
The verification confirms that all market IDs and denominations in the test support file are properly defined and unique:
- Each market (Spot, Derivative, Binary Options) has a unique market ID
- The denominations are consistent with the platform's token format:
- Native token:
inj - Peggy tokens:
peggy0x... - Factory tokens:
factory/...
- Native token:
The market definitions are used consistently within test files and there are no conflicting or duplicate market IDs across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the uniqueness and correctness of MarketIds and Denoms.
# Check for duplicate MarketIds
rg 'MarketId:\s+"(0x[a-f0-9]+)"' -r '$1' | sort | uniq -d
# Verify Denoms
rg 'BaseDenom:\s+"(.+)"' -r '$1'
rg 'QuoteDenom:\s+"(.+)"' -r '$1'
Length of output: 2133
Script:
#!/bin/bash
# Search for all market definitions to understand the context better
# Looking for complete market definitions with their IDs and denoms
# Search for complete spot market definitions
echo "=== Spot Markets ==="
ast-grep --pattern 'SpotMarket{
$$$
Ticker: $_,
BaseDenom: $_,
QuoteDenom: $_,
$$$
MarketId: $_,
$$$
}'
echo -e "\n=== Derivative Markets ==="
ast-grep --pattern 'DerivativeMarket{
$$$
Ticker: $_,
$$$
QuoteDenom: $_,
MarketId: $_,
$$$
}'
echo -e "\n=== Binary Options Markets ==="
ast-grep --pattern 'BinaryOptionsMarket{
$$$
Ticker: $_,
$$$
QuoteDenom: $_,
MarketId: $_,
$$$
}'
Length of output: 537
Script:
#!/bin/bash
# Search with exact struct initialization syntax
echo "=== Spot Markets ==="
ast-grep -l go --pattern 'marketInfo := exchangetypes.SpotMarket{
$$$
}'
echo -e "\n=== Derivative Markets ==="
ast-grep -l go --pattern 'marketInfo := exchangetypes.DerivativeMarket{
$$$
}'
echo -e "\n=== Binary Options Markets ==="
ast-grep -l go --pattern 'market := exchangetypes.BinaryOptionsMarket{
$$$
}'
# Also check for any other market ID references
echo -e "\n=== Market ID References ==="
rg "MarketId.*0x" -B2 -A2
Length of output: 15877
client/core/market.go (2)
171-189: Definition of BinaryOptionMarket struct is appropriate
The new BinaryOptionMarket struct is well-defined with all necessary fields and follows consistent naming conventions.
191-265: Ensure unit tests cover all new methods of BinaryOptionMarket
The BinaryOptionMarket type includes multiple new methods. Please ensure these methods are thoroughly tested to verify their correctness and prevent future regressions.
Would you like assistance in generating unit tests for these methods?
client/keyring/keyring.go (1)
1-496: Overall, the keyring implementation is robust and comprehensive
The keyring.go file provides a thorough implementation for managing various key types within the Cosmos SDK, with careful consideration for different configurations and secure handling of cryptographic materials. The use of helper functions and structured error handling enhances the maintainability and clarity of the code.
client/chain/markets_assistant.go (1)
235-237: Approve addition of AllBinaryOptionMarkets method
The AllBinaryOptionMarkets method is consistent with other market accessors and correctly returns the binary option markets.
| - name: Setup OS keystore | ||
| run: pass init keyring_test |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and verification for password store initialization
The current implementation doesn't handle potential failures in the prerequisite GPG key setup or verify successful initialization.
- - name: Setup OS keystore
- run: pass init keyring_test
+ - name: Setup OS keystore
+ run: |
+ if ! gpg --list-keys keyring_test > /dev/null 2>&1; then
+ echo "Error: GPG key 'keyring_test' not found"
+ exit 1
+ fi
+ pass init keyring_test
+ if ! pass git init > /dev/null 2>&1; then
+ echo "Error: Failed to initialize password store"
+ exit 1
+ fi📝 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.
| - name: Setup OS keystore | |
| run: pass init keyring_test | |
| - name: Setup OS keystore | |
| run: | | |
| if ! gpg --list-keys keyring_test > /dev/null 2>&1; then | |
| echo "Error: GPG key 'keyring_test' not found" | |
| exit 1 | |
| fi | |
| pass init keyring_test | |
| if ! pass git init > /dev/null 2>&1; then | |
| echo "Error: Failed to initialize password store" | |
| exit 1 | |
| fi |
| - name: Generate GPG key | ||
| run: " | ||
| echo \"%no-protection\nKey-Type: 1\nKey-Length: 4096\nSubkey-Type: 1\nSubkey-Length: 4096\nName-Comment: keyring_test\nExpire-Date: 0\" > genkey && gpg --gen-key --batch genkey" |
There was a problem hiding this comment.
Security and maintainability improvements needed for GPG key generation
Several concerns with the current implementation:
- The
%no-protectionflag makes the key vulnerable - The temporary
genkeyfile is not cleaned up - The multi-line command could be more readable
Consider this more secure and maintainable approach:
- - name: Generate GPG key
- run: "
- echo \"%no-protection\nKey-Type: 1\nKey-Length: 4096\nSubkey-Type: 1\nSubkey-Length: 4096\nName-Comment: keyring_test\nExpire-Date: 0\" > genkey && gpg --gen-key --batch genkey"
+ - name: Generate GPG key
+ run: |
+ cat > genkey <<EOF
+ Key-Type: 1
+ Key-Length: 4096
+ Subkey-Type: 1
+ Subkey-Length: 4096
+ Name-Comment: keyring_test
+ Expire-Date: 0
+ %commit
+ EOF
+ gpg --gen-key --batch genkey
+ rm genkey📝 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.
| - name: Generate GPG key | |
| run: " | |
| echo \"%no-protection\nKey-Type: 1\nKey-Length: 4096\nSubkey-Type: 1\nSubkey-Length: 4096\nName-Comment: keyring_test\nExpire-Date: 0\" > genkey && gpg --gen-key --batch genkey" | |
| - name: Generate GPG key | |
| run: | | |
| cat > genkey <<EOF | |
| Key-Type: 1 | |
| Key-Length: 4096 | |
| Subkey-Type: 1 | |
| Subkey-Length: 4096 | |
| Name-Comment: keyring_test | |
| Expire-Date: 0 | |
| %commit | |
| EOF | |
| gpg --gen-key --batch genkey | |
| rm genkey |
| // WithPrivKeyHex allows to specify a private key as plaintext hex. Insecure option, use for testing only. | ||
| // The package will create a virtual keyring holding that key, to meet all the interfaces. | ||
| func WithPrivKeyHex(v string) KeyConfigOpt { | ||
| return func(c *cosmosKeyConfig) error { | ||
| if v != "" { | ||
| c.PrivKeyHex = v | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Strengthen private key handling security.
The private key handling needs additional security measures:
- Add validation for private key format
- Implement secure memory handling for the private key
- Consider adding a mechanism to clear sensitive data after use
-// WithPrivKeyHex allows to specify a private key as plaintext hex. Insecure option, use for testing only.
-// The package will create a virtual keyring holding that key, to meet all the interfaces.
+// WithPrivKeyHex allows specification of a private key in hex format.
+// SECURITY WARNING: This is an insecure option that exposes private keys in plaintext.
+// Use only for testing. In production, use secure key storage mechanisms.
+// The package creates a virtual keyring to maintain interface compatibility.
func WithPrivKeyHex(v string) KeyConfigOpt {
return func(c *cosmosKeyConfig) error {
if v != "" {
+ if !isValidHex(v) {
+ return errors.New("invalid hex format for private key")
+ }
c.PrivKeyHex = v
}Committable suggestion skipped: line range outside the PR's diff.
| // WithMnemonic allows to specify a mnemonic pharse as plaintext. Insecure option, use for testing only. | ||
| // The package will create a virtual keyring to derive the keys and meet all the interfaces. | ||
| func WithMnemonic(v string) KeyConfigOpt { | ||
| return func(c *cosmosKeyConfig) error { | ||
| if v != "" { | ||
| if !bip39.IsMnemonicValid(v) { | ||
| err := errors.New("provided mnemonic is not a valid BIP39 mnemonic") | ||
| return err | ||
| } | ||
|
|
||
| c.Mnemonic = v | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve mnemonic validation and error handling.
The mnemonic handling can be improved in several ways:
- Add validation for mnemonic length and word count
- Improve error message with specific validation details
- Consider implementing secure memory handling for mnemonics
-// WithMnemonic allows to specify a mnemonic pharse as plaintext. Insecure option, use for testing only.
-// The package will create a virtual keyring to derive the keys and meet all the interfaces.
+// WithMnemonic allows specification of a BIP39 mnemonic phrase.
+// SECURITY WARNING: This is an insecure option that exposes mnemonics in plaintext.
+// Use only for testing. In production, use secure key storage mechanisms.
+// The package creates a virtual keyring for key derivation and interface compatibility.
func WithMnemonic(v string) KeyConfigOpt {
return func(c *cosmosKeyConfig) error {
if v != "" {
+ words := strings.Fields(v)
+ if len(words) != 12 && len(words) != 24 {
+ return errors.New("mnemonic must be 12 or 24 words")
+ }
if !bip39.IsMnemonicValid(v) {
- err := errors.New("provided mnemonic is not a valid BIP39 mnemonic")
+ err := errors.New("invalid BIP39 mnemonic: must be a valid set of words from the BIP39 wordlist")
return err
}
c.Mnemonic = vCommittable suggestion skipped: line range outside the PR's diff.
| // WithKeyPassphrase sets the passphrase for keyring files. Insecure option, use for testing only. | ||
| // The package will fallback to os.Stdin if this option was not provided, but pass is required. | ||
| func WithKeyPassphrase(v string) KeyConfigOpt { | ||
| return func(c *cosmosKeyConfig) error { | ||
| if v != "" { | ||
| c.KeyPassphrase = v | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance security measures for passphrase handling.
The passphrase handling has several security considerations:
- The warning about insecurity should be more prominent (e.g., using WARNING or SECURITY prefix)
- Consider implementing secure string handling to prevent passphrase exposure in memory
- Add basic validation for passphrase strength
-// WithKeyPassphrase sets the passphrase for keyring files. Insecure option, use for testing only.
-// The package will fallback to os.Stdin if this option was not provided, but pass is required.
+// WithKeyPassphrase sets the passphrase for keyring files.
+// SECURITY WARNING: This is an insecure option intended for testing only.
+// In production environments, always use os.Stdin for passphrase input.
+// The package will fallback to os.Stdin if this option was not provided.
func WithKeyPassphrase(v string) KeyConfigOpt {
return func(c *cosmosKeyConfig) error {
- if v != "" {
+ if len(v) < 8 {
+ return errors.New("passphrase must be at least 8 characters long")
+ }
+ if v != "" {
c.KeyPassphrase = v
}📝 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.
| // WithKeyPassphrase sets the passphrase for keyring files. Insecure option, use for testing only. | |
| // The package will fallback to os.Stdin if this option was not provided, but pass is required. | |
| func WithKeyPassphrase(v string) KeyConfigOpt { | |
| return func(c *cosmosKeyConfig) error { | |
| if v != "" { | |
| c.KeyPassphrase = v | |
| } | |
| return nil | |
| } | |
| } | |
| // WithKeyPassphrase sets the passphrase for keyring files. | |
| // SECURITY WARNING: This is an insecure option intended for testing only. | |
| // In production environments, always use os.Stdin for passphrase input. | |
| // The package will fallback to os.Stdin if this option was not provided. | |
| func WithKeyPassphrase(v string) KeyConfigOpt { | |
| return func(c *cosmosKeyConfig) error { | |
| if len(v) < 8 { | |
| return errors.New("passphrase must be at least 8 characters long") | |
| } | |
| if v != "" { | |
| c.KeyPassphrase = v | |
| } | |
| return nil | |
| } | |
| } |
| BaseDenom: "inj", | ||
| QuoteDenom: "peggy0x87aB3B4C8661e07D6372361211B96ed4Dc36B1B5", | ||
| MakerFeeRate: math.LegacyMustNewDecFromStr("-0.0001"), | ||
| TakerFeeRate: math.LegacyMustNewDecFromStr("0.001"), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor: Use sdk.MustNewDecFromStr instead of deprecated math.LegacyMustNewDecFromStr
The math.LegacyMustNewDecFromStr function is deprecated. It's recommended to use sdk.MustNewDecFromStr from the cosmos-sdk for better compatibility and future maintenance.
Apply this diff to update the function calls:
-import (
- "cosmossdk.io/math"
- // other imports
-)
+import (
+ sdk "github.com/cosmos/cosmos-sdk/types"
+ // other imports
+)
// In your struct initializations:
- MakerFeeRate: math.LegacyMustNewDecFromStr("-0.0001"),
+ MakerFeeRate: sdk.MustNewDecFromStr("-0.0001"),
// Repeat for all occurrences of math.LegacyMustNewDecFromStrEnsure that you have the correct import for the sdk package and that all instances are updated.
Also applies to: 192-192, 215-215, 241-241
| ExpirationTimestamp: 1708099200, | ||
| SettlementTimestamp: 1707099200, |
There was a problem hiding this comment.
Critical Issue: SettlementTimestamp precedes ExpirationTimestamp
In the createFirstMatchBetBinaryOptionsMarket function, the SettlementTimestamp (1707099200) is earlier than the ExpirationTimestamp (1708099200). Typically, the settlement occurs after the market expires. This discrepancy could lead to logical errors in market settlement.
Apply this diff to correct the timestamps:
func createFirstMatchBetBinaryOptionsMarket() *exchangetypes.BinaryOptionsMarket {
market := exchangetypes.BinaryOptionsMarket{
// ... other fields ...
- ExpirationTimestamp: 1708099200,
- SettlementTimestamp: 1707099200,
+ ExpirationTimestamp: 1707099200,
+ SettlementTimestamp: 1708099200,
// ... other fields ...
}
return &market
}Please verify that the timestamps reflect the intended market lifecycle.
📝 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.
| ExpirationTimestamp: 1708099200, | |
| SettlementTimestamp: 1707099200, | |
| ExpirationTimestamp: 1707099200, | |
| SettlementTimestamp: 1708099200, |
| tmpPhrase := randPhrase(64) | ||
| armored := cosmcrypto.EncryptArmorPrivKey(privKey, tmpPhrase, privKey.Type()) | ||
| err := kb.ImportPrivKey(name, armored, tmpPhrase) | ||
| if err != nil { | ||
| err = errors.Wrapf(ErrCosmosKeyringImportFailed, "failed to import privkey: %s", err.Error()) | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Handle errors when adding private keys to the keyring
In addFromPrivKey, after modifying randPhrase to return an error, ensure that errors from randPhrase are properly handled. This prevents unexpected panics and improves the robustness of the code.
See the updated code in the previous comment for the necessary changes.
| func randPhrase(size int) string { | ||
| buf := make([]byte, size) | ||
| if _, err := rand.Read(buf); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| return string(buf) |
There was a problem hiding this comment.
Use encoding for random passphrases instead of raw bytes
The randPhrase function converts raw random bytes directly into a string, which may result in invalid UTF-8 characters and potential errors. It's better to encode the random bytes using hex or base64 encoding to ensure the passphrase is a valid string.
Apply this diff to fix the issue:
-func randPhrase(size int) string {
+func randPhrase(size int) (string, error) {
buf := make([]byte, size)
if _, err := rand.Read(buf); err != nil {
- panic(err)
+ return "", err
}
- return string(buf)
+ return hex.EncodeToString(buf), nil
}Update the caller in addFromPrivKey to handle the error:
func addFromPrivKey(kb cosmkeyring.Keyring, name string, privKey cryptotypes.PrivKey) error {
- tmpPhrase := randPhrase(64)
+ tmpPhrase, err := randPhrase(64)
+ if err != nil {
+ return errors.Wrap(err, "failed to generate random passphrase")
+ }
armored := cosmcrypto.EncryptArmorPrivKey(privKey, tmpPhrase, privKey.Type())
- err := kb.ImportPrivKey(name, armored, tmpPhrase)
+ err = kb.ImportPrivKey(name, armored, tmpPhrase)
if err != nil {
err = errors.Wrapf(ErrCosmosKeyringImportFailed, "failed to import privkey: %s", err.Error())
return err
}
return nil
}📝 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.
| func randPhrase(size int) string { | |
| buf := make([]byte, size) | |
| if _, err := rand.Read(buf); err != nil { | |
| panic(err) | |
| } | |
| return string(buf) | |
| func randPhrase(size int) (string, error) { | |
| buf := make([]byte, size) | |
| if _, err := rand.Read(buf); err != nil { | |
| return "", err | |
| } | |
| return hex.EncodeToString(buf), nil | |
| } |
| func checkPrivkeyHexMatchesMnemonic(pkHex string, mnemonicDerivedPkBytes []byte) error { | ||
| pkBytesFromHex, err := hexToBytes(pkHex) | ||
| if err != nil { | ||
| err = errors.Wrapf(ErrHexFormatError, "failed to decode cosmos account privkey: %s", err.Error()) | ||
| return err | ||
| } | ||
|
|
||
| if !bytes.Equal(mnemonicDerivedPkBytes, pkBytesFromHex) { | ||
| err := errors.Wrap( | ||
| ErrPrivkeyConflict, | ||
| "both mnemonic and privkey hex options provided, but privkey doesn't match mnemonic", | ||
| ) | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Check for nil slices before comparing
In checkPrivkeyHexMatchesMnemonic, ensure that both mnemonicDerivedPkBytes and pkBytesFromHex are not nil before comparing them with bytes.Equal to avoid potential nil pointer dereference.
Apply this diff to add nil checks:
if pkBytesFromHex == nil || mnemonicDerivedPkBytes == nil {
return errors.New("private key bytes are nil")
}
if !bytes.Equal(mnemonicDerivedPkBytes, pkBytesFromHex) {
err := errors.Wrap(
ErrPrivkeyConflict,
"both mnemonic and privkey hex options provided, but privkey doesn't match mnemonic",
)
return err
}📝 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.
| func checkPrivkeyHexMatchesMnemonic(pkHex string, mnemonicDerivedPkBytes []byte) error { | |
| pkBytesFromHex, err := hexToBytes(pkHex) | |
| if err != nil { | |
| err = errors.Wrapf(ErrHexFormatError, "failed to decode cosmos account privkey: %s", err.Error()) | |
| return err | |
| } | |
| if !bytes.Equal(mnemonicDerivedPkBytes, pkBytesFromHex) { | |
| err := errors.Wrap( | |
| ErrPrivkeyConflict, | |
| "both mnemonic and privkey hex options provided, but privkey doesn't match mnemonic", | |
| ) | |
| return err | |
| } | |
| return nil | |
| } | |
| func checkPrivkeyHexMatchesMnemonic(pkHex string, mnemonicDerivedPkBytes []byte) error { | |
| pkBytesFromHex, err := hexToBytes(pkHex) | |
| if err != nil { | |
| err = errors.Wrapf(ErrHexFormatError, "failed to decode cosmos account privkey: %s", err.Error()) | |
| return err | |
| } | |
| if pkBytesFromHex == nil || mnemonicDerivedPkBytes == nil { | |
| return errors.New("private key bytes are nil") | |
| } | |
| if !bytes.Equal(mnemonicDerivedPkBytes, pkBytesFromHex) { | |
| err := errors.Wrap( | |
| ErrPrivkeyConflict, | |
| "both mnemonic and privkey hex options provided, but privkey doesn't match mnemonic", | |
| ) | |
| return err | |
| } | |
| return nil | |
| } |
Summary by CodeRabbit
Release Notes
New Features
BinaryOptionMarkettype with methods for managing binary options trading.GetStatusmethod to theTendermintClientinterface for querying node status.MarketsAssistantto support binary option markets and improved token retrieval methods.Bug Fixes
Chores
go.modto include new versions and remove indirect dependencies.Documentation