Feat/refactor markets tokens initialization #255
Conversation
…the TX in the mainpool if it has been rejected by the node during broadcast
…c_broadcast_before_await fix/check_tx_result_in_sync_broadcast_before_await
…ps to be able to read/download the file
…ved the test to use a temp test directory
…th_relative Fix/make OFAC list file path relative
…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
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve the deletion of several files and the restructuring of client interactions within the codebase. Key files removed include digital certificates and configuration files, which were previously used for secure communications and trading parameters. Additionally, the 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." 🪧 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: 13
🧹 Outside diff range and nitpick comments (30)
examples/exchange/oracle/2_Price/example.go (3)
19-22: Consider consistent error handling approachThe error handling here uses panic while other parts of the code use return with error printing. Consider adopting a consistent approach across the file.
- if err != nil { - panic(err) - } + if err != nil { + fmt.Println(err) + return + }
63-66: Maintain consistent error handlingConsider using the same error handling pattern as used with the chain client initialization.
- if err != nil { - panic(err) - } + if err != nil { + fmt.Println(err) + return + }
Line range hint
73-87: Avoid hardcoded market IDThe market ID is hardcoded which makes the example less maintainable and harder to understand. Consider using a more descriptive approach or adding comments explaining the market ID.
- market := marketsAssistant.AllDerivativeMarkets()["0x17ef48032cb24375ba7c2e39f384e56433bcab20cbee9a7357e4cba2eb00abe6"] + // Example using BTC-USDT-PERP market + const btcUsdtPerpMarketID = "0x17ef48032cb24375ba7c2e39f384e56433bcab20cbee9a7357e4cba2eb00abe6" + market := marketsAssistant.AllDerivativeMarkets()[btcUsdtPerpMarketID]examples/chain/exchange/25_MsgUpdateDerivativeMarket/example.go (1)
Line range hint
67-76: Enhance example documentationAs this is an example file, it would be helpful to:
- Add comments explaining the purpose of each market parameter
- Document the expected format and source of the marketId
- Include example response handling
+ // Example of updating a derivative market's parameters + // Market ID can be obtained from the blockchain using chainClient.GetDerivativeMarkets() msg := &exchangetypes.MsgUpdateDerivativeMarket{ Admin: senderAddress.String(), MarketId: "0x17ef48032cb24375ba7c2e39f384e56433bcab20cbee9a7357e4cba2eb00abe6", + // New ticker symbol for the market NewTicker: "INJ/USDT PERP 2", + // Minimum price change allowed for orders NewMinPriceTickSize: chainMinPriceTickSize, + // Minimum quantity change allowed for orders NewMinQuantityTickSize: chainMinQuantityTickSize, + // Minimum notional value (price * quantity) for orders NewMinNotional: chainMinNotional, + // Initial margin ratio for opening positions (40%) NewInitialMarginRatio: math.LegacyMustNewDecFromStr("0.4"), + // Maintenance margin ratio to prevent liquidation (8.5%) NewMaintenanceMarginRatio: math.LegacyMustNewDecFromStr("0.085"), }examples/chain/13_BroadcastMsgWithoutSimulation/example.go (1)
64-64: Consider adding context cancellationWhile the context creation is correct, consider adding a cancellation mechanism to prevent potential resource leaks.
-ctx := context.Background() +ctx, cancel := context.WithCancel(context.Background()) +defer cancel()examples/chain/exchange/16_MsgLiquidatePosition/example.go (1)
62-67: Consider adding context timeout for blockchain operationsWhile the transition to
chainclient.NewMarketsAssistantis good, the background context should have a timeout to prevent hanging in case of network issues.Consider adding a timeout to the context:
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()examples/chain/exchange/13_MsgInstantBinaryOptionsMarketLaunch/example.go (1)
Line range hint
63-78: Consider enhancing example to demonstrate blockchain data usage.While the removal of exchangeclient aligns with the PR's objectives, this example could better demonstrate the new architecture by:
- Fetching token metadata from the blockchain for precision calculation
- Using dynamic quote token resolution
- Adding comments explaining the new blockchain-first approach
This would make the example more valuable for users learning the new architecture.
examples/chain/exchange/4_MsgInstantPerpetualMarketLaunch/example.go (1)
69-69: Consider extracting the factory denom to a constantWhile using the factory denom format is correct, hardcoding it directly in the example might make maintenance more difficult. Consider extracting this to a constant or configuration value, possibly shared across examples.
+const ( + USDCFactoryDenom = "factory/inj17vytdwqczqz72j65saukplrktd4gyfme5agf6c/usdc" +) msg := &exchangetypes.MsgInstantPerpetualMarketLaunch{ Sender: senderAddress.String(), Ticker: "INJ/USDC PERP", - QuoteDenom: "factory/inj17vytdwqczqz72j65saukplrktd4gyfme5agf6c/usdc", + QuoteDenom: USDCFactoryDenom,examples/chain/exchange/5_MsgInstantExpiryFuturesMarketLaunch/example.go (1)
Line range hint
66-82: Enhance example documentation and error handling.As this is an example file demonstrating market launch functionality, it would benefit from:
- Comments explaining the market parameters and their significance
- Validation for network-specific configurations
- Documentation about required environment setup
Consider adding:
- Parameter documentation:
// Market Parameters Guide: // - Ticker: Trading pair identifier (e.g., "INJ/USDC FUT") // - OracleScaleFactor: Defines the scaling factor for oracle prices // - InitialMarginRatio: Required margin for opening positions // - MaintenanceMarginRatio: Minimum margin to maintain positions
- Network validation:
if network.ChainId == "testnet" { // Use testnet-specific configuration } else if network.ChainId == "mainnet" { // Use mainnet-specific configuration } else { panic("Unsupported network") }examples/chain/exchange/7_MsgCreateSpotMarketOrder/example.go (1)
62-67: Consider adding context timeout for market initializationWhile the new initialization looks good, using a bare background context could lead to hanging operations if the blockchain is unresponsive.
Consider adding a timeout:
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()examples/chain/exchange/6_MsgCreateSpotLimitOrder/example.go (1)
Line range hint
1-124: Consider adding documentation about the new market initialization approach.While the implementation is solid, it would be helpful to add comments explaining:
- The direct blockchain querying approach for market data
- Benefits of the new initialization method
- Any considerations when transitioning from the old approach
Example documentation to add at the start of the file:
package main +// This example demonstrates the new approach for market operations using chainclient, +// which retrieves market data directly from the blockchain instead of the Indexer. +// This provides more accurate and up-to-date market information while reducing +// external dependencies. + import (examples/chain/4_MsgBatchCreateSpotLimitOrders/example.go (2)
64-65: Consider using a context with timeout for production code.While
context.Background()works for this example, in production code, you might want to use a context with timeout to prevent potential hanging operations:-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()
65-69: Consider using more graceful error handling in example code.While
panicworks, it might be better to demonstrate proper error handling in example code to guide users toward best practices:- marketsAssistant, err := chainclient.NewMarketsAssistant(ctx, chainClient) - if err != nil { - panic(err) - } + marketsAssistant, err := chainclient.NewMarketsAssistant(ctx, chainClient) + if err != nil { + fmt.Printf("Failed to initialize markets assistant: %v\n", err) + return + }examples/chain/exchange/10_MsgCreateDerivativeLimitOrder/example.go (2)
Line range hint
69-71: Consider making market ID configurable.The market ID is currently hardcoded. Consider making it configurable through environment variables or command-line flags for better flexibility and maintainability.
- marketId := "0x4ca0f92fc28be0c9761326016b5a1a2177dd6375558365116b5bdda9abc229ce" + marketId := os.Getenv("MARKET_ID") + if marketId == "" { + marketId = "0x4ca0f92fc28be0c9761326016b5a1a2177dd6375558365116b5bdda9abc229ce" // default value + }
Line range hint
116-118: Consider implementing proper transaction confirmation.Using a fixed sleep duration (
time.Sleep(time.Second * 5)) is not reliable for waiting for transaction confirmation. Consider implementing a proper transaction confirmation mechanism.- time.Sleep(time.Second * 5) + // Wait for transaction confirmation with timeout + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + // Poll for transaction confirmation + ticker := time.NewTicker(time.Second) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return fmt.Errorf("timeout waiting for transaction confirmation") + case <-ticker.C: + // Add transaction confirmation check logic here + // Break the loop when confirmed + } + }examples/chain/exchange/11_MsgCreateDerivativeMarketOrder/example.go (1)
63-68: Add comments explaining the new blockchain-based market initializationWhile the implementation is correct, adding comments would help users understand the architectural change from Indexer to blockchain-based market data retrieval.
Add explanatory comments:
ctx := context.Background() +// Initialize markets assistant using blockchain data instead of Indexer +// This provides direct access to on-chain market information marketsAssistant, err := chainclient.NewMarketsAssistant(ctx, chainClient) if err != nil { panic(err) }examples/chain/5_MsgBatchCreateDerivativeLimitOrders/example.go (1)
64-69: Improve context management for better resource handling.While the transition to
chainclientlooks good, consider enhancing the context management:
- Use a cancellable context with timeout
- Ensure proper context propagation to subsequent operations
Consider this improvement:
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()examples/chain/authz/2_MsgExec/example.go (1)
80-84: Consider adding context timeout for safety.The markets assistant initialization looks good and aligns with the PR objectives of using blockchain data. However, consider adding a timeout to the context for better control over the initialization process.
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()examples/chain/exchange/9_MsgBatchUpdateOrders/example.go (2)
Line range hint
73-89: Consider improving example code maintainability.The spot order creation example uses hardcoded market IDs and values. While this works for demonstration, it could become outdated or confusing for users.
Consider:
- Adding comments explaining the market IDs
- Using named constants for better maintainability
- Adding a note about how users should obtain real market IDs
- smarketId := "0x0511ddc4e6586f3bfe1acb2dd905f8b8a82c97e1edaef654b12ca7e6031ca0fa" - samount := decimal.NewFromFloat(2) - sprice := decimal.NewFromFloat(22.5) + // Example market ID for INJ/USDT spot market + const EXAMPLE_SPOT_MARKET_ID = "0x0511ddc4e6586f3bfe1acb2dd905f8b8a82c97e1edaef654b12ca7e6031ca0fa" + const EXAMPLE_SPOT_ORDER_AMOUNT = 2 + const EXAMPLE_SPOT_ORDER_PRICE = 22.5 + + smarketId := EXAMPLE_SPOT_MARKET_ID + samount := decimal.NewFromFloat(EXAMPLE_SPOT_ORDER_AMOUNT) + sprice := decimal.NewFromFloat(EXAMPLE_SPOT_ORDER_PRICE)
Line range hint
91-105: Add parameter validation for order creation.The order creation lacks validation for critical parameters like price and quantity. While this is example code, it should demonstrate best practices.
Consider adding validation before order creation:
+ // Validate spot order parameters + if samount.LessThanOrEqual(decimal.Zero) { + panic("Invalid spot order amount") + } + if sprice.LessThanOrEqual(decimal.Zero) { + panic("Invalid spot order price") + } + spot_order := chainClient.CreateSpotOrder( defaultSubaccountID, &chainclient.SpotOrderData{Similarly for derivative orders:
+ // Validate derivative order parameters + if damount.LessThanOrEqual(decimal.Zero) { + panic("Invalid derivative order amount") + } + if dprice.LessThanOrEqual(decimal.Zero) { + panic("Invalid derivative order price") + } + if dleverage.LessThanOrEqual(decimal.Zero) { + panic("Invalid leverage") + } + derivative_order := chainClient.CreateDerivativeOrder(Also applies to: 107-123
client/chain/markets_assistant_test_support.go (2)
173-175: Consider safer decimal conversion alternativesThe use of
math.LegacyMustNewDecFromStrfor fee rates could panic if the string is malformed. Consider using the non-panicking alternativemath.LegacyNewDecFromStrwith proper error handling for more robust test data.Also applies to: 191-193, 215-217, 240-242
168-184: Document market configuration valuesConsider adding comments explaining the significance of specific market configuration values (e.g., tick sizes, notional values, fee rates) to help other developers understand the test data setup.
Also applies to: 186-202, 204-226
client/chain/markets_assistant_test.go (1)
95-159: Consider extracting token metadata setup to a helper function.The token metadata setup is quite lengthy and could be moved to a helper function to improve test readability and maintainability. This would also make it easier to reuse in other tests.
+ func createTestTokenMetadata() []core.TokenMetadata { + return []core.TokenMetadata{ + { + Address: "0x44C21afAaF20c270EBbF5914Cfc3b5022173FEB7", + // ... rest of the APE token metadata + }, + // ... rest of the tokens + } + } func TestMarketAssistantCreation(t *testing.T) { - tokensList := []core.TokenMetadata{ - { - Address: "0x44C21afAaF20c270EBbF5914Cfc3b5022173FEB7", - // ... rest of the token metadata - }, - // ... rest of the tokens - } + tokensList := createTestTokenMetadata()client/core/market.go (3)
171-189: LGTM! Consider adding field documentation.The
BinaryOptionMarketstruct is well-structured and includes all necessary fields for binary options trading. The use of a pointer forSettlementPriceappropriately handles the case when settlement price is not yet available.Consider adding godoc comments for the struct and its fields to improve code documentation.
191-231: Consider enhancing error handling in chain format conversions.The current implementation silently ignores potential errors from
LegacyNewDecFromStr. While this is consistent with existing patterns, consider explicit error handling for better debugging and reliability.Example improvement for
QuantityToChainFormat:-func (market BinaryOptionMarket) QuantityToChainFormat(humanReadableValue decimal.Decimal) sdkmath.LegacyDec { +func (market BinaryOptionMarket) QuantityToChainFormat(humanReadableValue decimal.Decimal) (sdkmath.LegacyDec, error) { chainFormattedValue := humanReadableValue quantizedValue := chainFormattedValue.DivRound(market.MinQuantityTickSize, 0).Mul(market.MinQuantityTickSize) - valueInChainFormat, _ := sdkmath.LegacyNewDecFromStr(quantizedValue.String()) + valueInChainFormat, err := sdkmath.LegacyNewDecFromStr(quantizedValue.String()) + if err != nil { + return sdkmath.LegacyDec{}, fmt.Errorf("failed to convert quantity to chain format: %w", err) + } - return valueInChainFormat + return valueInChainFormat, nil }
191-265: Consider reducing code duplication across market types.The conversion methods are nearly identical across
SpotMarket,DerivativeMarket, andBinaryOptionMarket. Consider extracting common logic into helper functions or using composition to reduce duplication.Example approach:
// Common base struct for shared functionality type BaseMarket struct { QuoteToken Token MinPriceTickSize decimal.Decimal MinQuantityTickSize decimal.Decimal } // Helper functions for common conversions func (base BaseMarket) baseQuantityToChainFormat(humanReadableValue decimal.Decimal) sdkmath.LegacyDec { // Common implementation } // BinaryOptionMarket can embed BaseMarket type BinaryOptionMarket struct { BaseMarket // Specific fields }client/core/market_test.go (1)
360-508: Consider adding edge case testsThe binary option market test suite covers the basic conversion scenarios but lacks tests for edge cases.
Consider adding tests for:
- Zero values
- Maximum/minimum values
- Invalid inputs
- Settlement scenarios (given the market has settlement timestamp)
client/chain/chain_test_support.go (1)
370-381: Consider reducing code duplication in market query methods.The same response handling pattern is repeated in
FetchChainSpotMarkets,FetchChainDerivativeMarkets, andFetchChainBinaryOptionsMarkets. Consider extracting this pattern into a generic helper function.+func getNextResponse[T any](responses *[]T) (*T, error) { + if len(*responses) > 0 { + response := (*responses)[0] + *responses = (*responses)[1:] + return &response, nil + } + var empty T + return &empty, 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 getNextResponse(&c.QuerySpotMarketsResponses) }Also applies to: 449-460, 564-575
client/chain/markets_assistant.go (2)
Line range hint
239-258: Avoid usingpanicininitializeTokensFromChainDenomsUsing
panicfor error handling is discouraged in library code as it can cause unwanted termination of the program using the library.Refactor the code to return errors instead of panicking:
func (assistant *MarketsAssistant) initializeTokensFromChainDenoms(ctx context.Context, chainClient ChainClient) error { // Existing code... if err != nil { - panic(err) + return err } // Existing code... + return nil }Ensure that all calling functions handle the returned error appropriately.
Line range hint
275-289: Potential issue withdecimalsdefault valueThe variable
decimalsis initialized to-1, which may lead to incorrect token decimal assignments if no denom units have an exponent greater than-1.Set a default value that accurately reflects the absence of exponent information or ensure that
decimalsis correctly assigned.Suggested change:
- var decimals int32 = -1 + var decimals int32 = 0This ensures that if no denom units are found, the token will have
0decimals, which may be more appropriate.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (36)
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/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)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)
💤 Files with no reviewable changes (9)
- 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
🔇 Additional comments (61)
examples/exchange/oracle/2_Price/example.go (2)
38-60: LGTM!
The client context and chain client initialization is well-structured with proper error handling.
68-71: 🛠️ Refactor suggestion
Maintain consistent error handling and verify exchange client usage
The error handling should be consistent with other parts of the code. Additionally, according to the PR objectives, we're moving away from using the exchange client.
- if err != nil {
- panic(err)
- }
+ if err != nil {
+ fmt.Println(err)
+ return
+ }Let's verify the exchange client usage across the codebase:
examples/chain/exchange/25_MsgUpdateDerivativeMarket/example.go (2)
9-9: LGTM: Import changes align with new architecture
The addition of rpchttp import and removal of context package aligns with the PR's objective to streamline client interactions.
63-65:
Critical: Avoid hardcoding token decimals
The hardcoded decimal value of 6 makes the code brittle and contradicts the PR's objective of retrieving data from the blockchain. Different tokens can have different decimal places, and hardcoding this value could lead to incorrect calculations.
Consider refactoring to fetch token decimals from the blockchain. Here's a suggested approach:
- chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6)))
- chainMinQuantityTickSize := minQuantityTickSize
- chainMinNotional := minNotional.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6)))
+ // Fetch quote token metadata from blockchain
+ quoteToken, err := chainClient.GetTokenMetadata(quoteTokenDenom)
+ if err != nil {
+ panic(err)
+ }
+ decimals := quoteToken.Decimals
+ chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), decimals))
+ chainMinQuantityTickSize := minQuantityTickSize
+ chainMinNotional := minNotional.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), decimals))Let's verify if other examples in the codebase have similar hardcoding:
examples/chain/exchange/24_MsgUpdateSpotMarket/example.go (2)
9-9: LGTM: Import changes align with architectural improvements
The import changes correctly support the transition from exchangeclient to direct blockchain interaction via chainclient, which aligns with the PR's objective of fetching market data directly from the blockchain.
Also applies to: 12-12, 14-14
63-67: 🛠️ Refactor suggestion
Document decimal assumptions and consider making them configurable
The calculations use hardcoded decimal values (6 and 18) which appear to be specific to USDC and INJ tokens respectively. This could cause issues if used with other token pairs that have different decimal places.
Consider:
- Adding comments explaining the decimal choices
- Making the decimal values configurable based on the actual tokens being used
- Documenting that this example is specific to INJ/USDC if that's the intent
Let's verify if these decimal values are consistent with other market examples:
Consider refactoring to make the decimals configurable:
-chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6)))
-chainMinPriceTickSize = chainMinPriceTickSize.Quo(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(18)))
-chainMinQuantityTickSize := minQuantityTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(18)))
-chainMinNotional := minNotional.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6)))
+// Define decimal places for the token pair
+const (
+ quoteTokenDecimals = 6 // USDC has 6 decimal places
+ baseTokenDecimals = 18 // INJ has 18 decimal places
+)
+
+// Adjust price tick size by quote token decimals and base token decimals
+chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), quoteTokenDecimals))
+chainMinPriceTickSize = chainMinPriceTickSize.Quo(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), baseTokenDecimals))
+
+// Adjust quantity tick size by base token decimals
+chainMinQuantityTickSize := minQuantityTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), baseTokenDecimals))
+
+// Adjust min notional by quote token decimals
+chainMinNotional := minNotional.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), quoteTokenDecimals))examples/chain/exchange/3_MsgInstantSpotMarketLaunch/example.go (3)
9-9: LGTM! Import changes align with architectural goals
The removal of exchangeclient and addition of rpchttp aligns with the PR's objective to interact directly with the blockchain instead of using the Indexer.
63-67: Verify hardcoded decimal precision values
The calculations now use fixed decimal values instead of getting them from token metadata:
- Price tick size: 6 decimals
- Quantity tick size: 18 decimals
- Notional: 6 decimals
Please confirm if these values are specifically intended for the INJ/USDC pair. If this example needs to work with other token pairs, consider making these values configurable.
✅ Verification successful
Let's search for more context about the decimal configurations and market standards in the codebase.
Based on the search results, I can now generate a final response since we've found evidence that these decimal values are specifically used in an INJ/USDC market example.
Hardcoded decimal values are appropriate for this specific example
The decimal precisions are correctly set for the INJ/USDC market example as shown in the code:
- The example explicitly creates an "INJ/USDC" market (line 71)
- The decimal values match the standard token decimals:
- USDC typically uses 6 decimals (used in price tick size and notional)
- INJ uses 18 decimals (used in quantity tick size)
Since this is a specific example for the INJ/USDC pair, the hardcoded values are appropriate here and don't need to be configurable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other market launch examples to verify decimal standards
rg -l "MsgInstantSpotMarketLaunch" | while read -r file; do
echo "=== $file ==="
rg "NewDecFromIntWithPrec.*\d+" "$file"
done
Length of output: 1434
Script:
#!/bin/bash
# Search for token decimals and market configurations
rg -A 5 -B 5 "USDC|INJ" examples/chain/exchange/3_MsgInstantSpotMarketLaunch/
# Look for any market configuration or decimal constants
rg -A 2 -B 2 "const.*decimal|decimal.*const" examples/chain/exchange/
# Search for any documentation about market decimals
fd README.md | xargs rg -i "decimal|tick.?size|notional"
Length of output: 1507
72-73: Document factory address format and verify USDC contract address
The hardcoded denominations make this example specific to INJ/USDC pair. Consider:
- Adding comments explaining the factory address format for USDC
- Verifying if this is the correct USDC contract address for the testnet
Consider adding a comment above the QuoteDenom assignment explaining the factory address format:
BaseDenom: "inj",
+// Factory denominations follow the format: factory/<contract_address>/<subdenom>
QuoteDenom: "factory/inj17vytdwqczqz72j65saukplrktd4gyfme5agf6c/usdc",✅ Verification successful
Factory denom format is correct, but add documentation for clarity
The factory denom format is correctly used across the codebase and follows the standard format factory/{creator_address}/{subdenom} as documented in the tokenfactory module. The same USDC factory address is consistently used across spot, perpetual, and futures market examples.
Consider adding this documentation:
BaseDenom: "inj",
+// Token factory denoms follow the format: factory/{creator_address}/{subdenom}
+// This represents the USDC token created by the factory contract
QuoteDenom: "factory/inj17vytdwqczqz72j65saukplrktd4gyfme5agf6c/usdc",🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of this USDC factory address to verify consistency
rg "factory/inj17vytdwqczqz72j65saukplrktd4gyfme5agf6c/usdc"
# Look for any documentation about factory addresses
fd -t f "README.md" | while read -r file; do
echo "=== $file ==="
rg -A 5 "factory.*denom" "$file"
done
Length of output: 792
Script:
#!/bin/bash
# Search for any documentation about factory denoms in Go files
rg -A 5 "factory.*denom|Factory.*Denom" --type go
# Search for any comments or documentation about USDC contract addresses
rg -B 2 -A 2 "usdc" --type go
# Look for any test files that might contain documentation about factory denoms
fd -e test.go | while read -r file; do
echo "=== $file ==="
rg -i "factory.*denom|usdc" "$file"
done
# Search for any constants or variables related to factory denoms
ast-grep --pattern 'const $_ = "factory/'
ast-grep --pattern 'var $_ = "factory/'
Length of output: 13852
examples/chain/13_BroadcastMsgWithoutSimulation/example.go (2)
8-8: LGTM! Import changes align with architectural improvements
The transition from exchangeclient to chainclient imports reflects the architectural improvement of retrieving market data directly from the blockchain instead of the Indexer.
Also applies to: 13-15
64-69: LGTM! Market initialization refactored successfully
The transition to using chainclient.NewMarketsAssistant aligns with the PR objectives and provides a more direct approach to market initialization.
examples/chain/exchange/16_MsgLiquidatePosition/example.go (1)
9-9: LGTM: Import changes align with the refactoring goals
The addition of rpchttp and common imports supports the transition to direct blockchain communication.
Also applies to: 16-16
examples/chain/exchange/13_MsgInstantBinaryOptionsMarketLaunch/example.go (3)
9-9: LGTM! Import changes align with architecture updates.
The addition of rpchttp and removal of exchangeclient imports align with the PR's objective to interact directly with the blockchain instead of using the Indexer.
78-78: 🛠️ Refactor suggestion
Make QuoteDenom configurable for better example reusability.
Hardcoding the QuoteDenom to a specific USDT address reduces the example's reusability and educational value. Consider:
- Making it a configurable parameter
- Adding comments explaining the token address format
- Providing examples with different quote tokens
Here's a suggested improvement:
- QuoteDenom: "peggy0xdAC17F958D2ee523a2206206994597C13D831ec7",
+ // QuoteDenom: Peggy-prefixed ERC20 address for USDT
+ QuoteDenom: os.Getenv("QUOTE_DENOM_ADDRESS"),Let's check if other examples use different quote tokens:
63-63:
Consider maintaining dynamic precision handling.
The hardcoded precision value of 6 reduces flexibility and could cause issues with tokens that have different decimal places. Consider retrieving the precision dynamically from the token metadata, which aligns better with the PR's objective of using blockchain data.
Let's verify if other tokens with different decimal places are used in the codebase:
examples/chain/exchange/4_MsgInstantPerpetualMarketLaunch/example.go (3)
9-9: LGTM: Appropriate import for blockchain communication
The addition of the cometbft RPC client import aligns with the PR's objective of retrieving data directly from the blockchain.
Line range hint 1-91: LGTM: Well-structured example demonstrating new initialization flow
The example effectively demonstrates the new market initialization flow using the blockchain directly, aligning with the PR's objectives. It includes proper error handling and provides a clear template for users.
63-63: ```shell
#!/bin/bash
Let's try different search patterns to find USDC related configurations and decimal places
Search for USDC token configurations
rg -i "usdc" -A 5 -B 5
Search for any decimal configurations in market or token setups
ast-grep --pattern 'Decimals = $_'
Search for any perpetual market configurations
rg -i "perpetual.*market.*launch|market.*perpetual.*launch" -A 5 -B 5
Look for any token decimal configurations in test files
rg -i "token.*decimal|decimal.*token" -A 3 -B 3
</details>
<details>
<summary>examples/chain/exchange/5_MsgInstantExpiryFuturesMarketLaunch/example.go (1)</summary>
`9-9`: **LGTM! Import changes align with the refactoring goals.**
The addition of `rpchttp` and removal of the context package properly reflect the transition from exchangeclient to direct blockchain interaction.
</details>
<details>
<summary>examples/chain/exchange/7_MsgCreateSpotMarketOrder/example.go (2)</summary>
`9-9`: **LGTM! Import changes align with the new architecture**
The transition from exchangeclient to chainclient is properly reflected in the imports.
Also applies to: 14-16
---
Line range hint `69-124`: **Enhance example documentation and robustness**
While the implementation is correct, consider these improvements:
1. Add comments explaining the workflow and expected outcomes
2. Consider making the market ID configurable or document its source
3. Replace the arbitrary sleep with proper transaction confirmation
Let's verify if this market ID is still valid:
</details>
<details>
<summary>examples/chain/exchange/6_MsgCreateSpotLimitOrder/example.go (2)</summary>
`9-9`: **LGTM! Import changes align with the refactoring goals.**
The updated imports correctly reflect the transition from `exchangeclient` to `chainclient` for market operations.
Also applies to: 14-16
---
`61-65`: **Verify market data retrieval functionality.**
The new markets assistant initialization looks good and aligns with the PR's goal of retrieving market data directly from the blockchain instead of the Indexer.
Let's verify the implementation of the new market data retrieval:
<details>
<summary>✅ Verification successful</summary>
Let me generate additional verification scripts to check the implementation details of the markets assistant:
---
Let me try one more verification to check the market data retrieval methods using a different pattern:
---
Based on the verification results, I can now provide the final response:
**Market data retrieval implementation verified successfully**
The new `chainclient.NewMarketsAssistant` initialization is consistently used across all example files and properly replaces the old exchange client approach. The implementation provides three initialization methods:
- Direct chain-based initialization (the one being used)
- Legacy exchange client initialization (maintained for backward compatibility)
- Combined initialization with all tokens
The consistent usage pattern across 13 example files and the clean struct definition confirms proper implementation of the market data retrieval functionality.
</details>
<details>
<summary>🏁 Scripts executed</summary>
The following scripts were executed for the analysis:
Script:
```shell
#!/bin/bash
# Description: Verify the new markets assistant implementation and its usage
# Look for any remaining references to old market data retrieval methods
# Check for any remaining exchangeclient market initialization
rg -l "NewMarketsAssistantInitializedFromChain|exchangeclient\.ExchangeClient"
# Verify consistent usage of new initialization pattern
ast-grep --pattern 'chainclient.NewMarketsAssistant($_, $_)'
Length of output: 2106
Script:
#!/bin/bash
# Check the implementation of markets assistant
rg -A 10 "type MarketsAssistant struct" client/chain/markets_assistant.go
# Check if there are any remaining references to exchange client in the implementation
rg "exchangeclient" client/chain/markets_assistant.go
# Look for market data retrieval methods in the new implementation
rg -A 5 "func \(.*MarketsAssistant\) Get" client/chain/markets_assistant.go
Length of output: 609
Script:
#!/bin/bash
# Check the implementation methods of MarketsAssistant
rg -A 5 "func \(m \*MarketsAssistant\)" client/chain/markets_assistant.go
# Check test file for usage patterns
rg -A 5 "func Test.*MarketsAssistant" client/chain/markets_assistant_test.go
# Look for any initialization or setup methods
rg "func New.*MarketsAssistant" client/chain/markets_assistant.go
Length of output: 607
examples/chain/4_MsgBatchCreateSpotLimitOrders/example.go (2)
9-16: LGTM! Import changes align with the architectural shift.
The imports correctly reflect the transition from exchangeclient to chainclient, and include necessary dependencies for the new implementation.
65-65: Verify blockchain querying implementation.
The PR objectives mention that market data should be retrieved directly from the blockchain rather than the Indexer. Let's verify this implementation:
✅ Verification successful
Let me search for more specific information about the MarketsAssistant implementation.
Let me search for the implementation of NewMarketsAssistant to understand how it initializes and fetches market data.
Let me check the implementation of initializeFromChain to see how it fetches market data from the blockchain.
Based on the code analysis so far, I can now provide a definitive response about the blockchain querying implementation.
Implementation correctly queries blockchain directly
The implementation in MarketsAssistant fetches market data directly from the blockchain through the chain client:
FetchChainSpotMarketsandFetchChainDerivativeMarketsin the chain client make direct gRPC calls to the blockchain's exchange query service- The implementation uses
exchangeQueryClient.SpotMarketsandexchangeQueryClient.DerivativeMarketswhich are blockchain RPC endpoints - No indexer dependencies are found in the market data retrieval flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of NewMarketsAssistant to verify it queries blockchain
ast-grep --pattern 'func NewMarketsAssistant($_, $_) {
$$$
}'
# Search for any remaining indexer references
rg -i "indexer"
Length of output: 5451
Script:
#!/bin/bash
# Search for MarketsAssistant struct and implementation
rg -A 10 "type MarketsAssistant struct"
# Search for market-related queries in the chain client
rg -A 5 "QuerySpotMarkets|QueryDerivativeMarkets|GetSpotMarket|GetDerivativeMarket"
Length of output: 59187
Script:
#!/bin/bash
# Search for NewMarketsAssistant implementation
rg -A 20 "func NewMarketsAssistant"
# Search for FetchChainSpotMarkets and FetchChainDerivativeMarkets implementation
rg -A 10 "func.*FetchChain(Spot|Derivative)Markets"
Length of output: 6864
Script:
#!/bin/bash
# Search for initializeFromChain implementation
rg -A 20 "func.*initializeFromChain"
# Search for exchangeQueryClient implementation to verify blockchain queries
rg -A 10 "type.*exchangeQueryClient struct"
Length of output: 1646
examples/chain/8_OfflineSigning/example.go (3)
10-16: LGTM! Import changes align with architectural shift
The removal of exchangeclient and addition of necessary imports for chainclient align well with the PR's objective of refactoring market initialization to use blockchain data directly.
Line range hint 82-120: LGTM! Clean integration with existing code
The new market initialization approach integrates seamlessly with the existing order creation and transaction handling logic. The example maintains clarity and serves as good documentation for the new blockchain-based market data retrieval.
75-80: Verify market data retrieval performance
The initialization now uses blockchain queries instead of the Indexer. While this provides more accurate data, it might impact performance.
Consider implementing caching mechanisms if the market data queries become a performance bottleneck. The blockchain queries might be slower than the previous Indexer-based approach.
examples/chain/exchange/10_MsgCreateDerivativeLimitOrder/example.go (1)
9-16: LGTM! Import changes align with the refactoring goals.
The imports have been properly updated to support the transition from exchangeclient to chainclient, maintaining only the necessary dependencies.
examples/chain/exchange/11_MsgCreateDerivativeMarketOrder/example.go (2)
9-9: LGTM! Import changes align with architectural shift
The addition of rpchttp and common imports supports the transition to blockchain-based data retrieval, maintaining consistency with the PR's objectives.
Also applies to: 16-16
63-68: Verify market assistant initialization across different networks
The initialization looks correct, but let's ensure it works reliably across different network conditions.
✅ Verification successful
Market assistant initialization is consistent and properly handled
The verification shows consistent initialization patterns across all examples:
- All instances use the same initialization pattern with proper context and chainClient
- Error handling is uniform with appropriate panic for initialization failures
- Implementation is consistent across different types of operations (spot, derivative, oracle, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent market assistant initialization patterns across examples
# and verify error handling in different network scenarios
# Check for consistent initialization pattern across examples
echo "Checking market assistant initialization patterns:"
rg -A 3 "NewMarketsAssistant\(ctx, chainClient\)" "examples/"
# Check for proper error handling in market-related operations
echo "Checking error handling patterns:"
ast-grep --pattern 'marketsAssistant, err := chainclient.NewMarketsAssistant($_, $_)
if err != nil {
$$$
}'
Length of output: 4708
examples/chain/5_MsgBatchCreateDerivativeLimitOrders/example.go (2)
9-9: LGTM! Import changes align with the architectural shift.
The addition of rpchttp and common imports properly supports the transition from exchangeclient to chainclient.
Also applies to: 16-16
65-65: Verify complete removal of indexer dependencies.
The implementation correctly uses chainclient.NewMarketsAssistant for market data retrieval. Let's verify that all indexer dependencies have been removed.
✅ Verification successful
Let me gather more specific information about any active indexer client usage.
Let me search for any remaining direct indexer imports or dependencies.
Based on the comprehensive search results, I can now provide the final response:
Indexer dependencies have been successfully removed from active code
The only remaining references to indexer are in generated protobuf files' comments, which are historical artifacts from code generation and do not represent active dependencies. The codebase has successfully transitioned to using chainclient for market data retrieval with no remaining active indexer dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining indexer dependencies in the codebase
# Search for indexer-related imports or usage
rg -i "indexer" --type go
# Search for specific indexer client patterns
ast-grep --pattern 'NewIndexerGrpcClient($$$)'
Length of output: 2109
Script:
#!/bin/bash
# Search for any remaining client imports or usage patterns
rg -i "indexerclient" --type go
# Check for any exchange client imports that might still use indexer
rg -i "exchangeclient" --type go
# Look for specific indexer-related function calls
ast-grep --pattern 'client.GetSpotMarkets($$$)'
ast-grep --pattern 'client.GetDerivativeMarkets($$$)'
Length of output: 57158
Script:
#!/bin/bash
# Search for direct indexer imports
rg -i "github.com/InjectiveLabs/injective-indexer" --type go
# Check for any remaining indexer-related types or interfaces
rg -i "indexerapi|indexergrpc" --type go
# Look for any indexer-related configuration
rg -i "indexer.*url|indexer.*endpoint" --type go
Length of output: 2189
examples/chain/authz/2_MsgExec/example.go (2)
13-18: LGTM! Import changes align with the architectural updates.
The import changes correctly reflect the shift from exchangeclient to chainclient as described in the PR objectives.
80-84: Verify complete migration from indexer.
Let's verify that there are no remaining references to the deprecated indexer or internal denom metadata in the codebase.
✅ Verification successful
Let me analyze the references found and generate additional verification scripts to ensure we have a complete picture.
Let me analyze the code further to verify the migration from indexer to chain client.
Based on the comprehensive analysis of the codebase, I can now provide a final response regarding the migration verification.
Migration to chain client is complete and verified.
The codebase shows:
- All indexer references are only in generated protobuf files (
.pb.go) - The new markets initialization uses
chainclient.NewMarketsAssistantcorrectly - No active usage of deprecated indexer client or internal denom metadata initialization
- Proper implementation of chain client interfaces and methods for market data retrieval
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to indexer or internal denom metadata
echo "Checking for indexer references..."
rg -i "indexer" --type go
echo "Checking for internal denom metadata references..."
rg -i "denom.*metadata.*ini" --type go
Length of output: 4077
Script:
#!/bin/bash
# Check for actual indexer usage in code (not just in generated files)
echo "Checking for indexer client usage..."
rg -i "indexerclient" --type go
echo "Checking for indexer imports..."
rg -i "import.*indexer" --type go
# Check for any remaining exchange client references
echo "Checking for exchange client usage..."
rg -i "exchangeclient" --type go
Length of output: 57329
Script:
#!/bin/bash
# Check for any remaining references to indexer in actual code (excluding generated files and examples)
echo "Checking for indexer references in source code..."
rg -i "indexer" --type go \
--glob '!{**/*.pb.go,examples/**}' \
-B 2 -A 2
# Check for any remaining references to internal denom metadata in actual code
echo "Checking for internal denom metadata in source code..."
rg -i "denom.*metadata.*ini" --type go \
--glob '!{**/*.pb.go,examples/**}' \
-B 2 -A 2
# Check for chain client usage in the codebase
echo "Checking for chain client usage..."
rg -i "chainclient" --type go \
--glob '!{**/*.pb.go,examples/**}' \
-B 2 -A 2
Length of output: 128220
examples/chain/1_LocalOrderHash/example.go (2)
9-11: LGTM! Import changes align with blockchain-direct approach.
The new imports support the transition from Indexer to direct blockchain interaction for market data retrieval.
62-67: LGTM! Markets initialization refactored to use blockchain directly.
The change properly implements the new initialization pattern using chainclient.NewMarketsAssistant, aligning with the PR objective of retrieving market data directly from the blockchain instead of the Indexer.
Let's verify the consistent usage of the new initialization pattern across the codebase:
✅ Verification successful
Let me verify the actual implementation pattern in these files to ensure consistency.
Markets initialization pattern is consistently implemented across the codebase
The verification confirms that:
- All examples consistently use
chainclient.NewMarketsAssistant(ctx, chainClient)pattern - No instances of the old
exchangeclientpattern remain - The implementation matches the reviewed code segment in terms of initialization and error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that exchangeclient is fully replaced with chainclient for markets initialization
# Check for any remaining exchangeclient usage in market initialization
rg -l "exchangeclient.*NewMarketsAssistant"
# Check for adoption of new chainclient pattern
rg -l "chainclient.*NewMarketsAssistant"
Length of output: 869
Script:
#!/bin/bash
# Check the actual implementation of NewMarketsAssistant usage
rg "NewMarketsAssistant" -A 2 -B 2
Length of output: 8084
examples/chain/exchange/9_MsgBatchUpdateOrders/example.go (2)
9-9: LGTM! Import changes align with the architectural shift.
The updated imports correctly reflect the transition from exchangeclient to chainclient for direct blockchain interaction.
Also applies to: 14-16
64-69: Verify market data retrieval performance.
The new initialization correctly uses chainclient to fetch market data directly from the blockchain instead of the indexer. This aligns with the PR objectives.
Let's verify the market data retrieval implementation:
✅ Verification successful
Based on the search results, I can see that the implementation of initializeFromChain in the MarketsAssistant properly fetches market data directly from the blockchain by:
- Loading official tokens from the network's token list
- Fetching active spot markets using
chainClient.FetchChainSpotMarkets - Properly handling token metadata and market information
The initialization is efficient and the error handling is appropriate. The market data retrieval is done in a single pass without redundant queries.
Market data retrieval implementation is efficient and reliable.
The new initialization correctly fetches market data directly from the blockchain with proper error handling and efficient data processing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of NewMarketsAssistant to ensure proper blockchain queries
ast-grep --pattern 'func NewMarketsAssistant(ctx context.Context, chainClient $_) ($_, $_) {
$$$
}'
Length of output: 548
Script:
#!/bin/bash
# Check the implementation of initializeFromChain to understand market data retrieval
ast-grep --pattern 'func ($_ *marketsAssistant) initializeFromChain(ctx context.Context, chainClient $_) error {
$$$
}'
# Also check the struct definition to understand what data is being stored
ast-grep --pattern 'type marketsAssistant struct {
$$$
}'
Length of output: 185
Script:
#!/bin/bash
# Let's try a broader search for the implementation using ripgrep
rg -A 20 "func.*initializeFromChain"
# Also search for marketsAssistant struct
rg -A 10 "type marketsAssistant struct"
# Search for any market data related queries
rg -A 5 "QueryMarket|GetMarket|FetchMarket"
Length of output: 192312
client/chain/markets_assistant_test_support.go (3)
4-8: LGTM! Imports align with blockchain-based market data approach
The new imports support the transition from Indexer to blockchain-based market data retrieval, as outlined in the PR objectives.
168-251: LGTM! Changes align with blockchain-based market data transition
The new market creation functions successfully implement the PR's objective of transitioning from Indexer to blockchain-based market data. The structures maintain consistency while providing comprehensive market configurations for different types of markets (spot, derivative, and binary options).
240-242: Verify zero fee rates for binary options market
The binary options market has 0% maker and taker fees, which differs from other markets. Please verify if this is intentional and consider adding a comment explaining this difference.
✅ Verification successful
Based on the search results, I can now generate the final response. The code shows that binary options markets have different fee structures compared to other market types, and there's evidence in the examples and test files that confirms this is intentional.
Zero fees are intentional for binary options markets
Binary options markets are designed to have 0% maker and taker fees, which differs from other market types:
- Spot markets have default 0.1% taker fees
- Derivative markets have default -0.01% maker and 0.1% taker fees
- Binary options specifically set both fees to 0%
This is a deliberate design choice for binary options trading. Consider adding a comment to explain this fee structure:
// Binary options markets have 0% maker and taker fees by design, unlike spot markets (0.1% taker)
// and derivative markets (-0.01% maker, 0.1% taker)
MakerFeeRate: math.LegacyMustNewDecFromStr("0"),
TakerFeeRate: math.LegacyMustNewDecFromStr("0"),🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other binary options markets to verify if zero fees are standard
rg -l "BinaryOptionsMarket.*MakerFeeRate.*0.*TakerFeeRate.*0" --type go
Length of output: 73
Script:
#!/bin/bash
# Let's search for binary options market fee rates with more context
rg "BinaryOptionsMarket.*Fee" -A 5 -B 5 --type go
# Also search for any comments or documentation about binary options fees
rg "binary.*options.*fee" -i --type go
Length of output: 23826
Script:
#!/bin/bash
# Let's search for any test files or documentation that might explain the fee structure
rg -B 3 -A 3 "BinaryOptionsMarket.*test" --type go
# Also search for any comments about binary options market fees in test files
rg "binary.*options.*fee.*test" -i --type go
# Search for any market creation or fee-related test cases
rg "NewBinaryOptionsMarket.*MakerFeeRate|TakerFeeRate" -A 5 --type go
Length of output: 77231
client/chain/markets_assistant_test.go (3)
5-5: LGTM! Import changes align with PR objectives.
The new imports support the transition from Indexer to blockchain for market data retrieval.
Also applies to: 11-17
173-241: LGTM! Comprehensive test coverage for the new blockchain-based initialization.
The test thoroughly validates:
- Spot market initialization
- Derivative market initialization
- Binary options market initialization
- Token mapping and verification
259-261: LGTM! Changes align with the transition to chain-based initialization.
The empty market responses are appropriate for testing token initialization without market data.
client/core/market.go (1)
171-265: Verify consistency with existing market implementations.
The implementation looks correct, but let's verify that it handles edge cases consistently with other market types.
✅ Verification successful
Based on the output, I can now provide a final response as there is sufficient evidence to verify the implementation consistency across market types.
Binary Option Market implementation is consistent with other market types
The implementation follows the same patterns as SpotMarket and DerivativeMarket:
- Identical method signatures and error handling patterns for all conversion methods
- Consistent decimal handling using
QuoteToken.Decimals - Proper quantization using
MinQuantityTickSizeandMinPriceTickSize - Matching implementation for margin and notional conversions
- Comprehensive test coverage with identical test cases and assertions
The code is well-aligned with existing market type implementations and handles edge cases consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare implementation patterns across market types
# Expected: Similar patterns for edge cases and error handling
# Check for consistent method signatures and error handling patterns
rg -A 2 "ToChainFormat|FromChainFormat" --type go
# Check for consistent decimal handling
rg -B 2 -A 2 "decimal\.New\(1, \w+\.Decimals\)" --type go
# Look for existing test cases that could be adapted
rg -l "TestMarket.*ChainFormat" --type go
Length of output: 21538
client/core/market_test.go (3)
397-410: Verify leverage handling for binary options
The TestCalculateMarginInChainFormat test for binary options includes leverage calculations. This seems unusual as binary options typically don't involve leverage.
#!/bin/bash
# Search for binary option leverage handling in the codebase
rg -A 5 "leverage.*binary.*option" 384-395: Review margin calculation for binary options
The margin calculation for binary options uses the same logic as derivative markets. Verify if binary options should handle margins differently given their unique nature (typically binary outcomes).
#!/bin/bash
# Search for binary option margin handling in the codebase
ast-grep --pattern 'func (m *BinaryOptionMarket) MarginToChainFormat'72-101: Review the market configuration values
The createBetBinaryOptionMarket function initializes a binary option market with zero maker and taker fees. This seems unusual compared to other market types that have non-zero fees.
Consider adding comments explaining why maker and taker fees are set to zero for binary option markets, or align the fee structure with other market types if this was unintentional.
✅ Verification successful
Zero fees for binary options market are intentionally different from other market types
The search results confirm that binary option markets are designed to have different fee structures compared to other market types:
- Spot/Derivative markets use non-zero fees: maker (-0.0001) and taker (0.001)
- Binary option markets intentionally use zero fees for both maker and taker
This is a valid design choice as binary options often have different fee models compared to traditional markets, while maintaining the same service provider fee (0.4) across all market types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other market configurations to verify fee structures
ast-grep --pattern 'makerFeeRate := decimal.$_'
Length of output: 71
Script:
#!/bin/bash
# Let's try a different approach to search for market configurations and fee structures
# First, let's search for files containing market-related test setup
fd "market.*test.*" --type f --exec rg -l "Fee|fee"
# Then, let's look for actual fee rate assignments in these files
rg "(?:maker|taker).*Fee.*:=.*decimal" -A 1 -B 1
Length of output: 3160
client/chain/chain_test_support.go (2)
37-41: LGTM: New fields align with PR objectives.
The new fields in MockChainClient struct appropriately support the refactoring of markets initialization, allowing the mock to simulate responses for different market types and network configuration.
774-776: LGTM: Simple getter implementation.
The GetNetwork method provides a clean way to access the network configuration, which aligns with the PR's objective of improving market and token initialization.
client/chain/chain.go (3)
296-296: LGTM: Good addition of GetNetwork() to the interface
The new method allows clients to retrieve network information, which aligns with the PR's objective of refactoring market initialization.
300-301: LGTM: Proper interface implementation check
Good practice to verify that chainClient implements the ChainClient interface at compile time.
2650-2653: LGTM: Clean implementation of GetNetwork()
The implementation is straightforward and correctly returns the network field. Since Network is likely an immutable value type, this is thread-safe.
client/chain/markets_assistant.go (7)
31-31: Addition of binaryOptionMarkets field to MarketsAssistant
The binaryOptionMarkets field has been added to store binary option markets within the MarketsAssistant struct. This enhancement allows for more comprehensive market data management.
36-40: Proper initialization of binaryOptionMarkets
In the newMarketsAssistant function, the binaryOptionMarkets map is correctly initialized, ensuring that binary option markets can be stored without issues.
161-166: Introduction of NewMarketsAssistant function
The new NewMarketsAssistant function initializes the MarketsAssistant and calls the initializeFromChain method. This streamlines the initialization process by fetching data directly from the chain.
430-432: Nil check before accessing SettlementPrice
The code correctly checks if marketInfo.SettlementPrice is not nil before accessing it. This prevents potential nil pointer dereferences.
219-238: Use of pointer receivers in methods
The methods AllTokens, AllTokensByDenom, AllSpotMarkets, AllDerivativeMarkets, and AllBinaryOptionMarkets now use pointer receivers. This change avoids copying the MarketsAssistant struct on method calls and allows modifications to the struct within methods if needed.
354-357: Missing tokens may lead to incomplete market data
When quoteTokenFound is false, the code skips adding that derivative market. This may result in incomplete market data if tokens are missing from tokensByDenom.
Consider fetching or adding missing tokens to tokensByDenom to ensure all markets are included. Alternatively, ensure that the token list is comprehensive.
To check for missing tokens, you can run:
#!/bin/bash
# Description: List all quote denoms that are missing from tokensByDenom.
rg 'GetQuoteDenom' -t go | awk '{print $NF}' | sort | uniq | while read denom; do
if ! grep -q "$denom" known_tokens.txt; then
echo "Missing token denom: $denom"
fi
doneReplace known_tokens.txt with the file containing all known token denoms.
44-47: Deprecated function NewMarketsAssistantInitializedFromChain
The function NewMarketsAssistantInitializedFromChain is marked as deprecated. Ensure that all usages of this function are updated to use the new NewMarketsAssistant to prevent reliance on outdated code.
To find all occurrences of the deprecated function, run the following script:
✅ Verification successful
The previous script's output seems incorrect. Let me try again with a more specific search.
No action needed - function only used in tests
The deprecated function NewMarketsAssistantInitializedFromChain is only used in its own test file (markets_assistant_test.go). Since test files are not part of the production code and are typically updated alongside their implementation, no further action is required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of the deprecated function in the codebase.
rg "NewMarketsAssistantInitializedFromChain"
Length of output: 698
Script:
#!/bin/bash
# Search for usages of the deprecated function
rg "NewMarketsAssistantInitializedFromChain" --type go
Length of output: 340
| senderAddress, cosmosKeyring, err := chainclient.InitCosmosKeyring( | ||
| os.Getenv("HOME")+"/.injectived", | ||
| "injectived", | ||
| "file", | ||
| "inj-user", | ||
| "12345678", | ||
| "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", // keyring will be used if pk not provided | ||
| false, | ||
| ) |
There was a problem hiding this comment.
Security: Remove hardcoded private key
The private key should not be hardcoded in the example code as it poses a security risk, even if it's just for demonstration purposes.
- "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", // keyring will be used if pk not provided
+ "", // provide your private key here or use keyring📝 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.
| senderAddress, cosmosKeyring, err := chainclient.InitCosmosKeyring( | |
| os.Getenv("HOME")+"/.injectived", | |
| "injectived", | |
| "file", | |
| "inj-user", | |
| "12345678", | |
| "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", // keyring will be used if pk not provided | |
| false, | |
| ) | |
| senderAddress, cosmosKeyring, err := chainclient.InitCosmosKeyring( | |
| os.Getenv("HOME")+"/.injectived", | |
| "injectived", | |
| "file", | |
| "inj-user", | |
| "12345678", | |
| "", // provide your private key here or use keyring | |
| false, | |
| ) |
| chainMinQuantityTickSize := minQuantityTickSize | ||
|
|
||
| msg := &exchangetypes.MsgInstantExpiryFuturesMarketLaunch{ | ||
| Sender: senderAddress.String(), | ||
| Ticker: "INJ/USDC FUT", | ||
| QuoteDenom: quoteToken.Denom, | ||
| QuoteDenom: "factory/inj17vytdwqczqz72j65saukplrktd4gyfme5agf6c/usdc", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding the factory denom address.
The hardcoded factory denom makes this example less reusable across different environments (testnet/mainnet) and could lead to confusion.
Consider these improvements:
-QuoteDenom: "factory/inj17vytdwqczqz72j65saukplrktd4gyfme5agf6c/usdc",
+// Option 1: Use environment variables
+QuoteDenom: os.Getenv("USDC_FACTORY_DENOM"),
+// Option 2: Use network-specific configuration
+QuoteDenom: network.GetFactoryDenom("usdc"),Also, add validation to ensure the denom format is correct:
if !strings.HasPrefix(quoteDenom, "factory/") {
panic("Invalid factory denom format")
}| minPriceTickSize := math.LegacyMustNewDecFromStr("0.01") | ||
| minQuantityTickSize := math.LegacyMustNewDecFromStr("0.001") | ||
|
|
||
| chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(quoteToken.Decimals))) | ||
| chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6))) |
There was a problem hiding this comment.
Consider making the precision configurable.
The hardcoded precision of 6 makes the code less flexible and might not work correctly for tokens with different decimal places. This could lead to incorrect price calculations for some markets.
Consider one of these approaches:
- Make precision configurable via parameters
- Fetch token metadata from the chain to determine the correct precision
-chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), int64(6)))
+// Option 1: Make configurable
+const quoteDenomPrecision = 6
+chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), quoteDenomPrecision))
+// Option 2: Fetch from chain (preferred)
+quoteToken, err := chainClient.GetTokenMetadata(msg.QuoteDenom)
+if err != nil {
+ panic(err)
+}
+chainMinPriceTickSize := minPriceTickSize.Mul(math.LegacyNewDecFromIntWithPrec(math.NewInt(1), quoteToken.Decimals))Committable suggestion skipped: line range outside the PR's diff.
| ctx := context.Background() | ||
| marketsAssistant, err := chainclient.NewMarketsAssistant(ctx, chainClient) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent client initialization patterns detected across examples
The verification reveals that many examples are still using the old exchangeclient.NewExchangeClient pattern instead of the new chainclient.NewMarketsAssistant. This inconsistency needs to be addressed:
- Found 58 examples still using
exchangeclient.NewExchangeClient - Only 13 examples using the new
chainclient.NewMarketsAssistant - The migration appears incomplete, particularly in the
examples/exchange/*directories
🔗 Analysis chain
LGTM! Verify consistent usage across examples.
The markets assistant initialization has been properly implemented using chainclient, with appropriate context creation and error handling.
Let's verify this pattern is consistently used across other examples:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent markets assistant initialization pattern across examples
# Expected: All examples should use chainclient.NewMarketsAssistant
# Search for any remaining exchangeclient usage
echo "Checking for any remaining exchangeclient usage:"
rg "exchangeclient\.NewExchangeClient" "examples/"
# Search for the new pattern
echo "Verifying new markets assistant initialization pattern:"
rg "chainclient\.NewMarketsAssistant" "examples/"
Length of output: 9758
client/core/market_test.go
Outdated
|
|
||
| marginDecimals := derivativeMarket.QuoteToken.Decimals | ||
| chainFormatMargin := expectedMargin.Mul(decimal.New(1, marginDecimals)) | ||
| humanReadablePrice := derivativeMarket.MarginFromChainFormat(sdkmath.LegacyMustNewDecFromStr(chainFormatMargin.String())) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent variable naming
Several test functions use humanReadablePrice for variables that represent margin or notional values.
Apply these diffs to fix the inconsistency:
- humanReadablePrice := derivativeMarket.MarginFromChainFormat(...)
+ humanReadableMargin := derivativeMarket.MarginFromChainFormat(...)
- humanReadablePrice := derivativeMarket.MarginFromExtendedChainFormat(...)
+ humanReadableMargin := derivativeMarket.MarginFromExtendedChainFormat(...)Also applies to: 462-462, 494-494, 505-505
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| senderAddress, cosmosKeyring, err := chainclient.InitCosmosKeyring( | ||
| os.Getenv("HOME")+"/.injectived", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use filepath.Join for constructing file paths
When building file paths, it's recommended to use filepath.Join to ensure cross-platform compatibility and proper handling of path separators.
Apply this change:
senderAddress, cosmosKeyring, err := chainclient.InitCosmosKeyring(
- os.Getenv("HOME")+"/.injectived",
+ filepath.Join(os.Getenv("HOME"), ".injectived"),
"injectived",📝 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.
| os.Getenv("HOME")+"/.injectived", | |
| filepath.Join(os.Getenv("HOME"), ".injectived"), |
| "12345678", | ||
| "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", // keyring will be used if pk not provided |
There was a problem hiding this comment.
Avoid hardcoding sensitive information like passwords and private keys
Hardcoding passwords and private keys in the code poses significant security risks. It's recommended to use environment variables or secure configuration management to handle sensitive data.
Apply this change:
senderAddress, cosmosKeyring, err := chainclient.InitCosmosKeyring(
...
- "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
false,
)Ensure that KEYRING_PASSWORD and PRIVATE_KEY are securely set as environment variables in your deployment environment.
📝 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.
| "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 |
| exchangeClient, err := exchangeclient.NewExchangeClient(network) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated exchangeClient with chainClient
The exchangeclient has been deprecated in favor of a more streamlined chainclient. You should replace the usage of exchangeClient with chainClient to align with the updated architecture.
Apply this change:
- exchangeClient, err := exchangeclient.NewExchangeClient(network)
- if err != nil {
- panic(err)
- }Refactor the code to use chainClient for streaming prices. Update the StreamPrices invocation accordingly.
Committable suggestion skipped: line range outside the PR's diff.
💡 Codebase verification
Moving from exchangeClient to chainClient requires alternative price streaming implementation
The verification shows that StreamPrices is only available in the exchangeClient and there's no equivalent functionality in the chainClient. This is a significant change that needs to be addressed.
- The
StreamPricesmethod is specifically implemented inclient/exchange/exchange.gofor the exchange client - No similar price streaming functionality was found in the chain client
- The example code relies on this streaming functionality for its core operation
🔗 Analysis chain
Verify the availability of StreamPrices method in chainClient
Since you're moving away from exchangeClient, ensure that the chainClient provides a similar StreamPrices method or an equivalent functionality.
Run the following script to verify if StreamPrices exists in chainclient:
This script searches for any method named StreamPrices within the chainclient package, helping you identify if such a method is available for use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence of the `StreamPrices` method in `chainclient`
# Search for the `StreamPrices` method definition in the chainclient package
ast-grep --pattern $'func ($_).StreamPrices($_, $_, $_, $_) { $$$ }' ./client/chain
Length of output: 84
Script:
#!/bin/bash
# Let's try a broader search for StreamPrices across the codebase
rg -l "StreamPrices"
# Also search for any price streaming related interfaces or methods
rg -l "PriceStream|StreamPrice"
# Let's also check the content of the example file to understand the context better
cat examples/exchange/oracle/1_StreamPrices/example.go
Length of output: 2890
Script:
#!/bin/bash
# Let's check the implementation of StreamPrices in exchange client
rg -A 10 "func.*StreamPrices" client/exchange/exchange.go
# Also check if there's any similar streaming functionality in chain client
rg -A 10 "func.*Stream.*Price" client/chain/
Length of output: 544
| } | ||
|
|
||
| assistant.spotMarkets[market.Id] = market | ||
| } | ||
|
|
||
| derivativeMarkets, err := chainClient.FetchChainDerivativeMarkets(ctx, "Active", nil, false) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, fullMarket := range derivativeMarkets.GetMarkets() { | ||
| marketInfo := fullMarket.GetMarket() | ||
|
|
||
| quoteToken, quoteTokenFound := assistant.tokensByDenom[marketInfo.GetQuoteDenom()] | ||
| if !quoteTokenFound { | ||
| // Ignore the market because it references a token that is not in the token list | ||
| continue | ||
| } | ||
|
|
||
| initialMarginRatio := decimal.RequireFromString(marketInfo.GetInitialMarginRatio().String()) | ||
| maintenanceMarginRatio := decimal.RequireFromString(marketInfo.MaintenanceMarginRatio.String()) | ||
| makerFeeRate := decimal.RequireFromString(marketInfo.GetMakerFeeRate().String()) | ||
| takerFeeRate := decimal.RequireFromString(marketInfo.GetTakerFeeRate().String()) | ||
| serviceProviderFee := decimal.RequireFromString(marketInfo.GetRelayerFeeShareRate().String()) | ||
| minPriceTickSize := decimal.RequireFromString(marketInfo.GetMinPriceTickSize().String()) | ||
| minQuantityTickSize := decimal.RequireFromString(marketInfo.GetMinQuantityTickSize().String()) | ||
| minNotional := decimal.RequireFromString(marketInfo.GetMinNotional().String()) | ||
|
|
||
| market := core.DerivativeMarket{ | ||
| Id: marketInfo.MarketId, | ||
| Status: marketInfo.GetMarketStatus().String(), | ||
| Ticker: marketInfo.GetTicker(), | ||
| OracleBase: marketInfo.OracleBase, | ||
| OracleQuote: marketInfo.OracleQuote, | ||
| OracleType: marketInfo.OracleType.String(), | ||
| OracleScaleFactor: marketInfo.GetOracleScaleFactor(), | ||
| InitialMarginRatio: initialMarginRatio, | ||
| MaintenanceMarginRatio: maintenanceMarginRatio, | ||
| QuoteToken: quoteToken, | ||
| MakerFeeRate: makerFeeRate, | ||
| TakerFeeRate: takerFeeRate, | ||
| ServiceProviderFee: serviceProviderFee, | ||
| MinPriceTickSize: minPriceTickSize, | ||
| MinQuantityTickSize: minQuantityTickSize, | ||
| MinNotional: minNotional, | ||
| } | ||
|
|
||
| assistant.derivativeMarkets[market.Id] = market | ||
| } | ||
|
|
||
| binaryOptionsMarkets, err := chainClient.FetchChainBinaryOptionsMarkets(ctx, "Active") | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, marketInfo := range binaryOptionsMarkets.GetMarkets() { | ||
| quoteToken, quoteTokenFound := assistant.tokensByDenom[marketInfo.GetQuoteDenom()] | ||
| if !quoteTokenFound { | ||
| // Ignore the market because it references a token that is not in the token list | ||
| continue | ||
| } | ||
|
|
||
| makerFeeRate := decimal.RequireFromString(marketInfo.GetMakerFeeRate().String()) | ||
| takerFeeRate := decimal.RequireFromString(marketInfo.GetTakerFeeRate().String()) | ||
| serviceProviderFee := decimal.RequireFromString(marketInfo.GetRelayerFeeShareRate().String()) | ||
| minPriceTickSize := decimal.RequireFromString(marketInfo.GetMinPriceTickSize().String()) | ||
| minQuantityTickSize := decimal.RequireFromString(marketInfo.GetMinQuantityTickSize().String()) | ||
| minNotional := decimal.RequireFromString(marketInfo.GetMinNotional().String()) | ||
|
|
||
| market := core.BinaryOptionMarket{ | ||
| Id: marketInfo.MarketId, | ||
| Status: marketInfo.GetMarketStatus().String(), | ||
| Ticker: marketInfo.GetTicker(), | ||
| OracleSymbol: marketInfo.OracleSymbol, | ||
| OracleProvider: marketInfo.OracleProvider, | ||
| OracleType: marketInfo.OracleType.String(), | ||
| OracleScaleFactor: marketInfo.GetOracleScaleFactor(), | ||
| ExpirationTimestamp: marketInfo.ExpirationTimestamp, | ||
| SettlementTimestamp: marketInfo.SettlementTimestamp, | ||
| QuoteToken: quoteToken, | ||
| MakerFeeRate: makerFeeRate, | ||
| TakerFeeRate: takerFeeRate, | ||
| ServiceProviderFee: serviceProviderFee, | ||
| MinPriceTickSize: minPriceTickSize, | ||
| MinQuantityTickSize: minQuantityTickSize, | ||
| MinNotional: minNotional, | ||
| } | ||
|
|
||
| if marketInfo.SettlementPrice != nil { | ||
| settlementPrice := decimal.RequireFromString(marketInfo.SettlementPrice.String()) | ||
| market.SettlementPrice = &settlementPrice | ||
| } | ||
|
|
||
| assistant.binaryOptionMarkets[market.Id] = market | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent error handling in initializeFromChain
The initializeFromChain function handles errors by returning them, which is good practice. Ensure that all functions called within initializeFromChain also follow this pattern to maintain consistency.
Verify that all internal function calls within initializeFromChain return errors instead of panicking, and handle those errors appropriately.
| func NewMarketsAssistantWithAllTokens(ctx context.Context, exchangeClient exchange.ExchangeClient, chainClient ChainClient) (MarketsAssistant, error) { | ||
| assistant := newMarketsAssistant() | ||
| assistant.initializeTokensFromChainDenoms(ctx, chainClient) | ||
| err := assistant.initializeFromChain(ctx, chainClient) | ||
|
|
||
| return assistant, nil | ||
| return assistant, err |
There was a problem hiding this comment.
Error handling in NewMarketsAssistantWithAllTokens
The NewMarketsAssistantWithAllTokens function does not handle potential errors from initializeTokensFromChainDenoms, which currently uses panic for error reporting. It's important to propagate errors instead of panicking to prevent unexpected termination of applications using this library.
Modify initializeTokensFromChainDenoms to return an error and handle it appropriately in NewMarketsAssistantWithAllTokens.
Update the function signature and error handling:
func NewMarketsAssistantWithAllTokens(ctx context.Context, exchangeClient exchange.ExchangeClient, chainClient ChainClient) (MarketsAssistant, error) {
assistant := newMarketsAssistant()
- assistant.initializeTokensFromChainDenoms(ctx, chainClient)
+ err := assistant.initializeTokensFromChainDenoms(ctx, chainClient)
+ if err != nil {
+ return assistant, err
+ }
err := assistant.initializeFromChain(ctx, chainClient)
return assistant, err
}Also, update initializeTokensFromChainDenoms 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 code...
+ return nil
}Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| "github.com/cosmos/cosmos-sdk/types/query" | ||
| banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" | ||
| "github.com/shopspring/decimal" | ||
|
|
||
| "github.com/InjectiveLabs/sdk-go/client/core" |
There was a problem hiding this comment.
What if we establish a special order of imports: 1) standard libs 2) 3d party libs 3) injective sdk-go libs
There was a problem hiding this comment.
Yes, that is the standard I am using
There was a problem hiding this comment.
@PavelInjective do you mean to consider the "cosmos-sdk" dependencies as sdk-go libs?
There was a problem hiding this comment.
yeah. i thought about it. but we can leave as it is.
| SettlementPrice *decimal.Decimal | ||
| } | ||
|
|
||
| func (market BinaryOptionMarket) QuantityToChainFormat(humanReadableValue decimal.Decimal) sdkmath.LegacyDec { |
There was a problem hiding this comment.
why don't we replace QuantityToChainFormat and PriceToChainFormat with one more general function?
There was a problem hiding this comment.
Because depending on whether you are translating price, quantity or notional the calculation is different, and this functions abstract the user from the complexity
client/core/market_test.go
Outdated
| func TestConvertQuantityToChainFormatForBinaryOptionMarket(t *testing.T) { | ||
| derivativeMarket := createBetBinaryOptionMarket() | ||
| originalQuantity := decimal.RequireFromString("123.456789") | ||
|
|
||
| chainValue := derivativeMarket.QuantityToChainFormat(originalQuantity) | ||
| quantizedValue := originalQuantity.DivRound(derivativeMarket.MinQuantityTickSize, 0).Mul(derivativeMarket.MinQuantityTickSize) | ||
| quantizedChainFormatValue := sdkmath.LegacyMustNewDecFromStr(quantizedValue.String()) | ||
|
|
||
| assert.Assert(t, quantizedChainFormatValue.Equal(chainValue)) | ||
| } | ||
|
|
||
| func TestConvertPriceToChainFormatForBinaryOptionMarket(t *testing.T) { | ||
| derivativeMarket := createBetBinaryOptionMarket() | ||
| originalPrice := decimal.RequireFromString("123.456789") | ||
|
|
||
| chainValue := derivativeMarket.PriceToChainFormat(originalPrice) | ||
| priceDecimals := derivativeMarket.QuoteToken.Decimals | ||
| expectedValue := originalPrice.Mul(decimal.New(1, priceDecimals)) | ||
| quantizedValue := expectedValue.DivRound(derivativeMarket.MinPriceTickSize, 0).Mul(derivativeMarket.MinPriceTickSize) | ||
| quantizedChainFormatValue := sdkmath.LegacyMustNewDecFromStr(quantizedValue.String()) | ||
|
|
||
| assert.Assert(t, quantizedChainFormatValue.Equal(chainValue)) | ||
| } | ||
|
|
||
| func TestConvertMarginToChainFormatForBinaryOptionMarket(t *testing.T) { | ||
| derivativeMarket := createBetBinaryOptionMarket() | ||
| originalPrice := decimal.RequireFromString("123.456789") | ||
|
|
||
| chainValue := derivativeMarket.MarginToChainFormat(originalPrice) | ||
| marginDecimals := derivativeMarket.QuoteToken.Decimals | ||
| expectedValue := originalPrice.Mul(decimal.New(1, marginDecimals)) | ||
| quantizedValue := expectedValue.DivRound(derivativeMarket.MinQuantityTickSize, 0).Mul(derivativeMarket.MinQuantityTickSize) | ||
| quantizedChainFormatValue := sdkmath.LegacyMustNewDecFromStr(quantizedValue.String()) | ||
|
|
||
| assert.Assert(t, quantizedChainFormatValue.Equal(chainValue)) | ||
| } | ||
|
|
||
| func TestCalculateMarginInChainFormatForBinaryOptionMarket(t *testing.T) { | ||
| derivativeMarket := createBetBinaryOptionMarket() | ||
| originalQuantity := decimal.RequireFromString("10") | ||
| originalPrice := decimal.RequireFromString("123.456789") | ||
| originalLeverage := decimal.RequireFromString("2.5") | ||
|
|
||
| chainValue := derivativeMarket.CalculateMarginInChainFormat(originalQuantity, originalPrice, originalLeverage) | ||
| decimals := derivativeMarket.QuoteToken.Decimals | ||
| expectedValue := originalQuantity.Mul(originalPrice).Div(originalLeverage).Mul(decimal.New(1, decimals)) | ||
| quantizedValue := expectedValue.DivRound(derivativeMarket.MinQuantityTickSize, 0).Mul(derivativeMarket.MinQuantityTickSize) | ||
| legacyDecimalQuantizedValue := sdkmath.LegacyMustNewDecFromStr(quantizedValue.String()) | ||
|
|
||
| assert.Assert(t, chainValue.Equal(legacyDecimalQuantizedValue)) | ||
| } |
There was a problem hiding this comment.
maybe a good suggestion
|
Sorry @PavelInjective, I could not get what the suggestion is in the last comment ("maybe a good suggestion") |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
client/chain/ofac.go (2)
Line range hint
1-93: Consider enhancing thread safety and caching mechanism.The current implementation could benefit from the following improvements:
- Thread-safe updates to the OFAC list
- Caching with expiration to avoid frequent disk reads
- Periodic background updates of the OFAC list
Here's a suggested enhancement:
type OfacChecker struct { ofacListPath string - ofacList map[string]bool + ofacList atomic.Value // holds map[string]bool + lastUpdate time.Time + mutex sync.RWMutex } func (oc *OfacChecker) loadOfacList() error { + oc.mutex.Lock() + defer oc.mutex.Unlock() file, err := os.ReadFile(oc.ofacListPath) if err != nil { return err } var list []string err = json.Unmarshal(file, &list) if err != nil { return err } newList := make(map[string]bool) for _, item := range list { newList[item] = true } - oc.ofacList = newList + oc.ofacList.Store(newList) + oc.lastUpdate = time.Now() return nil } func (oc *OfacChecker) IsBlacklisted(address string) bool { - return oc.ofacList[address] + if list, ok := oc.ofacList.Load().(map[string]bool); ok { + return list[address] + } + return false }
Line range hint
39-58: Enhance error handling in DownloadOfacList function.The function should handle edge cases like:
- Timeout for HTTP requests
- Maximum file size limits
- Response content type verification
- Temporary file usage for atomic updates
Here's a suggested enhancement:
func DownloadOfacList() error { + client := &http.Client{ + Timeout: 30 * time.Second, + } - resp, err := http.Get(DefaultOfacListURL) + resp, err := client.Get(DefaultOfacListURL) if err != nil { return fmt.Errorf("failed to download OFAC list: %w", err) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { return fmt.Errorf("failed to download OFAC list, status code: %d", resp.StatusCode) } + if ct := resp.Header.Get("Content-Type"); !strings.Contains(ct, "application/json") { + return fmt.Errorf("unexpected content type: %s", ct) + } + // Create temp file for atomic update + tmpFile, err := os.CreateTemp(OfacListPath, "ofac.*.tmp") + if err != nil { + return fmt.Errorf("failed to create temp file: %w", err) + } + tmpName := tmpFile.Name() + defer os.Remove(tmpName) - if err := os.MkdirAll(OfacListPath, 0755); err != nil { + if err := os.MkdirAll(OfacListPath, 0755); err != nil { return fmt.Errorf("failed to create directory: %w", err) } - outFile, err := os.Create(GetOfacListPath()) - if err != nil { - return err - } - defer outFile.Close() - _, err = io.Copy(outFile, resp.Body) + // Limit the file size to prevent DOS + _, err = io.Copy(tmpFile, io.LimitReader(resp.Body, 10<<20)) // 10MB limit if err != nil { - return err + return fmt.Errorf("failed to write OFAC list: %w", err) } - _, err = outFile.WriteString("\n") + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("failed to close temp file: %w", err) + } + // Atomic rename + if err := os.Rename(tmpName, GetOfacListPath()); err != nil { + return fmt.Errorf("failed to update OFAC list: %w", err) + } return nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
client/chain/ofac.go(1 hunks)injective_data/ofac.json(1 hunks)
🔇 Additional comments (3)
client/chain/ofac.go (1)
13-13: Verify the new OFAC list URL endpoint.
The URL structure has changed to include 'refs/heads/master/json'. Let's verify that this endpoint is accessible and returns the expected data format.
✅ Verification successful
New OFAC list URL is accessible and valid. No references to the old URL pattern found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new OFAC list URL is accessible and returns valid JSON
# Expected: HTTP 200 status code and valid JSON response
echo "Checking URL accessibility and response format..."
curl -s -o /dev/null -w "%{http_code}" "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofac.json"
echo -e "\nValidating JSON format..."
curl -s "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofac.json" | jq empty
# Also check if there are any references to the old URL pattern that need updating
echo -e "\nChecking for other references to the old URL pattern..."
rg -l "InjectiveLabs/injective-lists/master/wallets/ofac.json"
Length of output: 640
injective_data/ofac.json (2)
1-157: Verify compliance with official OFAC SDN list.
Given the critical nature of OFAC compliance:
- Ensure these addresses match the official OFAC SDN (Specially Designated Nationals) list
- Implement a process to track and document changes to this list
- Consider adding a comment header with the last update date and source reference
Would you like me to help create a GitHub issue to track the implementation of these compliance recommendations?
1-157: JSON structure and address format validation passed.
All Ethereum addresses follow the correct format (0x + 40 hex characters) and the JSON array is properly structured.
Run the following script to double-check the address format and JSON validity:
✅ Verification successful
JSON structure and address format validation confirmed.
All addresses are correctly formatted, and the JSON array is properly structured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Validate JSON structure and Ethereum address format
# Test 1: Verify JSON syntax
jq '.' injective_data/ofac.json > /dev/null
echo "JSON syntax validation result: $?"
# Test 2: Verify all addresses match Ethereum format
jq -r '.[]' injective_data/ofac.json | while read addr; do
if ! [[ $addr =~ ^0x[a-fA-F0-9]{40}$ ]]; then
echo "Invalid address format: $addr"
exit 1
fi
done
echo "Address format validation result: $?"
Length of output: 13654
… of the file after the update
fix/ofac_list_url'
… 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
ofac.jsonlistSummary by CodeRabbit
Summary by CodeRabbit
New Features
BinaryOptionMarkettype with methods for handling binary options.GetNetworkmethod to retrieve network information.MarketsAssistantto support binary option markets.Bug Fixes
Chores
chainclient.Documentation
MarketsAssistantfunctionalities.