[CP-127] add exchange v2 support #263
Conversation
WalkthroughThis update introduces a significant refactor and expansion of the codebase, particularly in the EVM module and exchange types. The EVM module receives a comprehensive implementation of transaction, log, error, parameter, and utility types, enabling robust Ethereum compatibility. The exchange types are streamlined, with many legacy methods and types removed, new concurrency-safe limits, and improved validation and utility functions. Several deprecated message types and proposal constructors are removed, and configuration files are modernized. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExchangeTypes
participant EVMTypes
participant CosmosSDK
User->>ExchangeTypes: Submit order / message
ExchangeTypes->>ExchangeTypes: Validate input, check limits, deduplicate
ExchangeTypes->>EVMTypes: (if EVM tx) Convert/validate Ethereum tx
EVMTypes->>CosmosSDK: Register codecs, handle tx, log, error types
CosmosSDK-->>User: Respond with tx result or error
sequenceDiagram
participant EVMTypes
participant Ethereum
participant CosmosSDK
EVMTypes->>Ethereum: Convert Cosmos tx to Ethereum tx format
Ethereum-->>EVMTypes: Return tx data, logs, errors
EVMTypes->>CosmosSDK: Register interfaces, pack/unpack Any, handle params
CosmosSDK-->>EVMTypes: Provide context for validation and execution
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-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
…at/add_exchange_v2_support
…at/add_exchange_v2_support
|
Is this stale? |
No, it is just work in progress, not stale |
…r` branch of injective-core (future v1.16)
…ctive core (for v1.16)
… endpoint (the /websocket destination is added internally now)
… human readable values in their config
…w reduce margin ratio for derivative markets
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (34)
chain/erc20/types/codec.go (1)
13-14: Exported constant lacks a doc-comment
golangci-lint(withgolint/revive) will flag exported identifiers that don’t start with a comment matching the identifier’s name.
Add a short sentence to avoid lint noise.-const ModuleName = "erc20" +// ModuleName defines the ERC-20 module’s name. +const ModuleName = "erc20"chain/exchange/types/errors.go (1)
53-54: Prefer user-friendly wording forErrMarginsRelationmessage
The error message currently embeds Go-style field names (MaintenanceMarginRatio,InitialMarginRatio, …). End-users seldom see or understand struct field names; consider re-phrasing with plain English, e.g.
"maintenance margin ratio must be lower than initial margin ratio, which must not exceed reduce margin ratio".
This keeps the API surface unchanged while improving UX and documentation.chain/exchange/types/fee_validation.go (1)
37-44: Move “test-only” helper behind a build tag
ValidateMakerWithTakerFeeAndDiscountsis annotated as “Test Code Only (for v1 tests)”, yet it ships in production builds.
A lightweight way to keep the helper while avoiding dead-code detectors and API exposure:-// Test Code Only (for v1 tests) +//go:build testutils +// +build testutils +// Test Code Only (for v1 tests)Then place the file in a
*_testutils.goor similar file.
This keeps the symbol out of release binaries but available to tests.chain/evm/types/events.go (1)
3-24: Exported constants lack doc comments – may trigger linters
golint,revive, and similar tools usually require a comment for every exported identifier. Adding a short description avoids CI noise:-const ( - EventTypeEthereumTx = TypeMsgEthereumTx +// EventTypeEthereumTx is emitted for every processed Ethereum-style transaction. +const ( + EventTypeEthereumTx = TypeMsgEthereumTxRepeat for the remaining exported constants as needed.
chain/exchange/types/market_admin.go (1)
4-13: Bit-flag enumeration is correct but can be simplified
Using1 << iotaon every line works, yet the idiomatic pattern sets it once and relies on blank identifiers:-const ( - TickerPerm = 1 << iota - MinPriceTickSizePerm = 1 << iota - MinQuantityTickSizePerm = 1 << iota - MinNotionalPerm = 1 << iota - InitialMarginRatioPerm = 1 << iota - MaintenanceMarginRatioPerm = 1 << iota - ReduceMarginRatioPerm = 1 << iota +const ( + _ = 1 << iota // skip value 1<<0 if unused + TickerPerm + MinPriceTickSizePerm + MinQuantityTickSizePerm + MinNotionalPerm + InitialMarginRatioPerm + MaintenanceMarginRatioPerm + ReduceMarginRatioPerm )Same values, less repetition and slightly clearer intent.
chain/exchange/types/key.go (3)
104-108: Consider removing commented-out code instead of preserving itRather than commenting out the legacy prefixes, it would be better to completely remove them if they're no longer used. Commented-out code can lead to confusion and maintenance issues over time.
- // old legacy prefixes - // SpotConditionalMarketOrdersPrefix = []byte{0x70} // prefix for a key to save conditional spot market orders: marketID + direction + triggerPrice + orderHash ⇒ spotMarketOrder - // SpotConditionalMarketOrdersIndexPrefix = []byte{0x71} // prefix for a key to save conditional spot market orders index: marketID + direction + subaccountID + orderHash ⇒ triggerPrice - // SpotConditionalLimitOrdersPrefix = []byte{0x72} // prefix for a key to save conditional spot limit orders: marketID + direction + triggerPrice + orderHash ⇒ spotLimitOrder - // SpotConditionalLimitOrdersIndexPrefix = []byte{0x73} // prefix for a key to save conditional spot limit orders index: marketID + direction + subaccountID + orderHash ⇒ triggerPrice
123-124: Add comments to describe the purpose of the new prefixesThe new prefixes lack descriptive comments explaining their purpose and usage. Adding comments similar to the other prefix constants would improve code readability and maintainability.
- MarketBalanceKey = []byte{0x84} // key for each key to a MarketBalance - OrderExpirationsPrefix = []byte{0x85} // prefix to store order expirations - OrderExpirationMarketsPrefix = []byte{0x86} // prefix to store markets with order expirations + MarketBalanceKey = []byte{0x84} // key for each key to a MarketBalance + OrderExpirationsPrefix = []byte{0x85} // prefix to store order expirations by block number and market ID + OrderExpirationMarketsPrefix = []byte{0x86} // prefix to store markets with order expirations by block number
570-590: Well-implemented key construction functionsThe new key construction functions follow the pattern of other similar functions in the file. They properly allocate buffers of the right size for efficiency and use consistent naming conventions.
Consider adding a function to parse these keys back into their components for completeness:
func ParseOrderExpirationPrefix(key []byte) (blockNumber int64, marketID common.Hash) { prefixLen := len(OrderExpirationsPrefix) blockNumberBz := key[prefixLen : prefixLen+Uint64BytesLen] marketIDBz := key[prefixLen+Uint64BytesLen:] return int64(sdk.BigEndianToUint64(blockNumberBz)), common.BytesToHash(marketIDBz) }.golangci.yml (1)
58-64: Consider documenting reasons for path exclusionsThe extensive list of excluded paths might make it difficult to know which parts of the codebase are not being linted. Consider adding comments to explain why these directories are excluded to help future maintainers understand the decisions.
paths: - - chain - - exchange - - injective_data - - proto - - client/keyring/testdata - - client/chain/chain_test_support.go + - chain # Excluded because: [add reason here] + - exchange # Excluded because: [add reason here] + - injective_data # Excluded because: [add reason here] + - proto # Generated code + - client/keyring/testdata # Test data + - client/chain/chain_test_support.go # Test support codechain/evm/types/logs.go (2)
30-47:Validaterejects pending-transaction logsA block height of 0 is legitimate for pending logs, yet line 62 treats it as an error. This prevents relaying unconfirmed txs through the API.
Consider either:
- Allowing
BlockNumber == 0whenRemoved == false, or- Removing the check entirely and relying on upstream validation.
91-100: Return an empty slice instead ofnilfor zero logs
NewLogsFromEthreturnsnilwhen the input slice is empty. Callers that range over the result or append to it will have to add an extra nil-check. Returning an empty slice avoids this foot-gun and follows Go conventions.- if len(ethlogs) == 0 { - return nil - } - logs := make([]*Log, 0, len(ethlogs)) + logs := make([]*Log, 0, len(ethlogs)) + if len(ethlogs) == 0 { + return logs + }chain/exchange/types/common_utils.go (1)
311-342: Duplicate-detection helpers can be consolidated with genericsThree almost-identical functions repeat the same pattern. Go 1.18 generics allow a single, type-agnostic helper, reducing code surface and maintenance.
// HasDuplicates reports whether f(x) produces the same key for any two elements. func HasDuplicates[T any, K comparable](slice []T, key func(T) K) bool { seen := make(map[K]struct{}) for _, item := range slice { k := key(item) if _, ok := seen[k]; ok { return true } seen[k] = struct{}{} } return false } // examples _ = HasDuplicates[string, string](denoms, func(s string) string { return s }) _ = HasDuplicates[string, common.Hash](hashes, common.HexToHash) _ = HasDuplicates[sdk.Coin, string](coins, func(c sdk.Coin) string { return c.Denom })Migrating would remove ~40 lines while keeping behaviour intact.
chain/evm/types/chain_config.go (1)
96-110: Escaping local variables repeatedly is memory-inefficient
getTimeValue/getBlockValuecreate a local, take its address, and return it.
While Go’s escape analysis makes this safe, you repeatedly allocate identical*big.Int/uint64values.You can avoid the extra allocations:
-func getTimeValue(time *sdkmath.Int) *uint64 { - ... - t := time.BigIntMut().Uint64() - return &t -} +func getTimeValue(tInt *sdkmath.Int) *uint64 { + if tInt == nil || tInt.IsNegative() { + return nil + } + v := tInt.BigIntMut().Uint64() + return &v +}Same for
getBlockValue.chain/evm/types/eth.go (1)
39-48: Infinite recursion guard & nil handling inUnmarshal
tx.UnmarshalBinary(dst)is the method of the embedded*ethtypes.Transaction,
so recursion is fine, buttx.Transactionmight still benilwhen
len(dst)==0, forcing the caller to remember this edge case.Consider normalising with:
if len(dst) == 0 { tx.Transaction = nil return nil } tx.Transaction = new(ethtypes.Transaction) return tx.Transaction.UnmarshalBinary(dst)chain/evm/types/tx_data.go (2)
78-98:DeriveChainID– slight readability & overflow improvementThe current implementation uses shadowed variables and manual subtraction.
A clearer, constant-free version prevents unsigned underflow:func DeriveChainID(v *big.Int) *big.Int { if v == nil || v.Sign() <= 0 { return nil } vv := new(big.Int).Set(v) switch vv.Uint64() { case 27, 28: return new(big.Int) // zero => no EIP-155 } // (v - 35) / 2 when v ≥ 35 vv.Sub(vv, big.NewInt(35)) return vv.Div(vv, big.NewInt(2)) }Purely cosmetic but reduces cognitive load.
50-68: Factory ignoresethtypes.BlobTxType(EIP-4844)Geth already defines
BlobTxType(0x03). If/when the EVM module accepts those
transactions, the factory will misinterpret them asLegacyTx.Consider future-proofing:
case ethtypes.BlobTxType: return nil, errorsmod.Wrap(ErrUnsupportedTxType, "EIP-4844 blob tx not supported yet")A clear error is better than silent mis-parsing.
chain/exchange/types/authz_exchange_generic.go (1)
42-81: Consider a map instead of an exhaustive switch forURL()The 30-line switch is repetitive and grows maintenance overhead each time a message is added/removed. A table-driven approach keeps the mapping declarative and reduces the risk of mismatched constants.
var msgTypeToURL = map[MsgType]string{ MsgTypeDeposit: "/injective.exchange.v1beta1.MsgDeposit", MsgTypeWithdraw: "/injective.exchange.v1beta1.MsgWithdraw", // ... } func (m MsgType) URL() string { if url, ok := msgTypeToURL[m]; ok { return url } return "UNKNOWN" }chain/evm/types/params.go (2)
17-19: Comment contradicts constant value
DefaultAllowUnprotectedTxsis set totrue, yet the comment says “rejects all unprotected txs (true = allow, false = reject)”. Either flip the boolean or adjust the wording.-// DefaultAllowUnprotectedTxs rejects all unprotected txs (true = allow, false = reject) +// DefaultAllowUnprotectedTxs controls acceptance of unprotected transactions (true = allow, false = reject)
60-75: Boolean validation adds little value
ValidateBoolmerely asserts the field is of typebool, which will always be true for a properly-generated struct. The extra indirection obscures the intent and bloats theValidatemethod.Consider removing these checks unless the param types can actually vary at runtime.
chain/evm/types/key.go (1)
65-77: Validate indices before casting touint64
ObjectGasUsedKeyandObjectBloomKeycoerceint→uint64without bounds checking.
If a caller accidentally passes a negativetxIndex/msgIndex, it will be converted to a huge positiveuint64, corrupting the key-space silently.Consider guarding the inputs:
-func ObjectGasUsedKey(txIndex int) []byte { +func ObjectGasUsedKey(txIndex int) []byte { + if txIndex < 0 { + panic("negative txIndex in ObjectGasUsedKey") + } … } -func ObjectBloomKey(txIndex, msgIndex int) []byte { +func ObjectBloomKey(txIndex, msgIndex int) []byte { + if txIndex < 0 || msgIndex < 0 { + panic("negative index in ObjectBloomKey") + } … }Alternatives: return an error instead of panicking, depending on how these helpers are used.
chain/evm/types/access_list_tx.go (1)
167-180:SetSignatureValuesmay lose the originalChainIDon nil inputWhen
chainIDisnilthe method leavestx.ChainIDunchanged, which is fine.
However, callers must remember to supply the value explicitly; otherwise,Validate()will later fail with “chain ID must be present”. Consider making the failure explicit here or documenting the contract in a comment to avoid confusion.chain/evm/types/legacy_tx.go (1)
146-156:SetSignatureValuessilently ignoreschainIDUnlike
AccessListTx, the legacy signer derives the chain-id fromv, yet callers might still pass it in.
Because the parameter is discarded (_), consider:
- Documenting this behaviour in a comment, or
- Returning an error if a non-nil
chainIDis supplied to avoid accidental misuse.Minor, but it helps future maintainers.
chain/evm/types/dynamic_fee_tx.go (1)
198-205: Minor wording & consistency nitError message strings miss the verb “be”:
-return errorsmod.Wrap(ErrInvalidGasCap, "gas tip cap cannot nil") +return errorsmod.Wrap(ErrInvalidGasCap, "gas tip cap cannot be nil")Apply the same fix for the gas-fee-cap branch below.
chain/evm/types/msg.go (1)
332-336: Builder type assertion may hide unsupported clientsWhen the assertion to
authtx.ExtensionOptionsTxBuilderfails, the error text
is generic ("unsupported builder"). Consider including the concrete type
received to aid debugging:return nil, fmt.Errorf("unsupported builder type %T", b)chain/evm/types/errors.go (1)
238-254:NewVMErrorWithRet: nilcausepath loses contextWhen
abi.UnpackRevertfails, the function setse.errbut leaves
e.causenil, so downstream ABCI logs lack module-level context.
Consider always settingcause, e.g.:if errUnpack == nil { ... -} else { - e.err = errors.New(e.vmErr) - e.cause = errorsmod.Wrap(ErrVMExecution, e.toJSON()) +} else { + e.err = errors.New(e.vmErr) + e.cause = errorsmod.Wrap(ErrVMExecution, e.toJSON()) }chain/evm/types/utils.go (2)
21-24: Spelling/grammar nit – “pratical” → “practical”.Tiny typo in the comment can be fixed to keep docs polished.
-// Because priority is `int64` while price is `big.Int`, it's necessary to scale down the range to keep it more pratical. +// Because priority is `int64` while price is `big.Int`, it's necessary to scale down the range to keep it more practical.
226-231: Guard against nil parameters inEffectiveGasPrice.
baseFee,feeCap, ortipCapbeingnilwould currently panic insidemath.BigMin/Add.
If this helper might ever be called with unset fields (e.g. during unit tests or partially-filled structs) add a short nil-check to returnbig.NewInt(0)or an error.chain/exchange/types/market.go (1)
64-112: Validate decimals before exponentiation to avoid hugeDecallocations.
math.LegacyNewDec(10).Power(uint64(decimals))can allocate very large numbers (and be slow) when an unexpecteddecimals> 18 sneaks in.
A cheap safeguard inside each converter avoids hidden panics:+const maxSupportedDecimals = 18 // matches Cosmos-SDK Limit + func PriceFromChainFormat(price math.LegacyDec, baseDecimals, quoteDecimals uint32) math.LegacyDec { - if price.IsNil() { + if price.IsNil() { return price } + if baseDecimals > maxSupportedDecimals || quoteDecimals > maxSupportedDecimals { + return math.LegacyZeroDec() // or panic / error – pick what suits the API + }Apply the same bound check to
Quantity*andNotional*helpers.chain/exchange/types/msgs.go (3)
95-106: Interface naming & placement
UpdateSpotMarketMessageis handy, but living in this monolithic file makes discovery harder.
Consider extracting all update-market message helpers & shared validation into a smallspot_update.goto improve locality.
113-122:ValidateAddresshelper: return wrapped error for context.Right now the original SDK error is lost once wrapped again at call-sites. Returning
errorsmod.Wrap(err, ...)directly from the helper preserves stack/context and removes boiler-plate.func ValidateAddress(address string) error { - _, err := sdk.AccAddressFromBech32(address) - return err + if _, err := sdk.AccAddressFromBech32(address); err != nil { + return errorsmod.Wrap(err, "invalid bech32 address") + } + return nil }
1037-1064: Constructor overload explosionTwo almost-identical helpers create binary-option market orders. Instead of duplicating, expose one function that takes an optional
scaleFactor(e.g.*uint32) or feed the factor directly (caller can passmarket.OracleScaleFactor).This removes one copy-pasted block and trims maintenance.
chain/exchange/types/derivative_orders.go (3)
13-28: Heavy code duplication forCid()helpers
DerivativeOrder,DerivativeLimitOrder, andDerivativeMarketOrderall forward toOrderInfo.GetCid(). This repetition invites future drift and bloats the API surface.Consider embedding a common interface (e.g.
HasOrderInfo) or just useo.OrderInfo.GetCid()at call-sites.-type DerivativeCID interface{ Cid() string } - -func (m *DerivativeLimitOrder) Cid() string { return m.OrderInfo.GetCid() } -func (o *DerivativeMarketOrder) Cid() string { return o.OrderInfo.GetCid() } -func (m *DerivativeOrder) Cid() string { return m.OrderInfo.GetCid() } +type derivativeWithInfo interface{ GetOrderInfo() *OrderInfo } + +func cid(of derivativeWithInfo) string { return of.GetOrderInfo().GetCid() }
29-39: Same duplication pattern forPrice()The three identical
Price()wrappers suffer the same maintenance overhead noted above. Folding them behind one helper (or embeddingOrderInfoin the proto) would reduce noise.
41-51: Same duplication pattern forSubaccountID()Again, three copies of identical code. Refactor as suggested to keep the file lean.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (35)
chain/auction/types/query.pb.gois excluded by!**/*.pb.gochain/erc20/types/erc20.pb.gois excluded by!**/*.pb.gochain/erc20/types/events.pb.gois excluded by!**/*.pb.gochain/erc20/types/genesis.pb.gois excluded by!**/*.pb.gochain/erc20/types/params.pb.gois excluded by!**/*.pb.gochain/erc20/types/query.pb.gois excluded by!**/*.pb.gochain/erc20/types/tx.pb.gois excluded by!**/*.pb.gochain/evm/types/access_tuple.pb.gois excluded by!**/*.pb.gochain/evm/types/chain_config.pb.gois excluded by!**/*.pb.gochain/evm/types/events.pb.gois excluded by!**/*.pb.gochain/evm/types/genesis.pb.gois excluded by!**/*.pb.gochain/evm/types/log.pb.gois excluded by!**/*.pb.gochain/evm/types/params.pb.gois excluded by!**/*.pb.gochain/evm/types/query.pb.gois excluded by!**/*.pb.gochain/evm/types/state.pb.gois excluded by!**/*.pb.gochain/evm/types/trace_config.pb.gois excluded by!**/*.pb.gochain/evm/types/transaction_logs.pb.gois excluded by!**/*.pb.gochain/evm/types/tx.pb.gois excluded by!**/*.pb.gochain/evm/types/tx_result.pb.gois excluded by!**/*.pb.gochain/exchange/types/authz.pb.gois excluded by!**/*.pb.gochain/exchange/types/exchange.pb.gois excluded by!**/*.pb.gochain/exchange/types/tx.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/authz.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/events.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/exchange.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/genesis.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/market.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/order.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/orderbook.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/proposal.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/query.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/tx.pb.gois excluded by!**/*.pb.gochain/stream/types/v2/query.pb.gois excluded by!**/*.pb.gochain/txfees/types/txfees.pb.gois excluded by!**/*.pb.gochain/types/indexer.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (47)
.github/workflows/golangci-lint.yml(1 hunks).golangci.yml(1 hunks)Makefile(4 hunks)auth_vote/authz_vote.go(1 hunks)chain/crypto/ethsecp256k1/ethsecp256k1.go(2 hunks)chain/crypto/hd/algorithm.go(3 hunks)chain/crypto/hd/hd_path.go(1 hunks)chain/erc20/types/codec.go(1 hunks)chain/evm/types/access_list.go(1 hunks)chain/evm/types/access_list_tx.go(1 hunks)chain/evm/types/chain_config.go(1 hunks)chain/evm/types/codec.go(1 hunks)chain/evm/types/dynamic_fee_tx.go(1 hunks)chain/evm/types/errors.go(1 hunks)chain/evm/types/eth.go(1 hunks)chain/evm/types/events.go(1 hunks)chain/evm/types/key.go(1 hunks)chain/evm/types/legacy_tx.go(1 hunks)chain/evm/types/logs.go(1 hunks)chain/evm/types/msg.go(1 hunks)chain/evm/types/params.go(1 hunks)chain/evm/types/storage.go(1 hunks)chain/evm/types/tx.go(1 hunks)chain/evm/types/tx_data.go(1 hunks)chain/evm/types/utils.go(1 hunks)chain/exchange/types/authz_common.go(3 hunks)chain/exchange/types/authz_derivative.go(5 hunks)chain/exchange/types/authz_exchange_generic.go(1 hunks)chain/exchange/types/authz_spot.go(5 hunks)chain/exchange/types/codec.go(1 hunks)chain/exchange/types/common_order.go(4 hunks)chain/exchange/types/common_utils.go(2 hunks)chain/exchange/types/deposit.go(0 hunks)chain/exchange/types/derivative_orders.go(2 hunks)chain/exchange/types/errors.go(3 hunks)chain/exchange/types/exchange.go(1 hunks)chain/exchange/types/fee_discounts.go(0 hunks)chain/exchange/types/fee_validation.go(2 hunks)chain/exchange/types/genesis.go(0 hunks)chain/exchange/types/key.go(4 hunks)chain/exchange/types/market.go(1 hunks)chain/exchange/types/market_admin.go(1 hunks)chain/exchange/types/msgs.go(14 hunks)chain/exchange/types/msgs_validation.go(1 hunks)chain/exchange/types/params.go(18 hunks)chain/exchange/types/positions.go(1 hunks)chain/exchange/types/proposal.go(3 hunks)
💤 Files with no reviewable changes (3)
- chain/exchange/types/deposit.go
- chain/exchange/types/genesis.go
- chain/exchange/types/fee_discounts.go
🧰 Additional context used
🧬 Code Graph Analysis (21)
auth_vote/authz_vote.go (1)
client/common/network.go (1)
LoadNetwork(53-202)
chain/exchange/types/authz_spot.go (1)
chain/exchange/types/authz_common.go (1)
AuthorizedMarketsLimit(42-46)
chain/exchange/types/msgs_validation.go (4)
chain/exchange/types/msgs.go (1)
UpdateSpotMarketMessage(95-106)chain/exchange/types/errors.go (7)
ErrInvalidAddress(91-91)ErrMarketInvalid(13-13)ErrBadField(12-12)ErrInvalidTicker(19-19)ErrInvalidPriceTickSize(42-42)ErrInvalidQuantityTickSize(43-43)ErrInvalidNotional(108-108)chain/exchange/types/common_utils.go (1)
IsHexHash(110-120)chain/exchange/types/params.go (3)
MaxTickerLength(40-40)ValidateTickSize(370-410)ValidateMinNotional(412-427)
chain/crypto/ethsecp256k1/ethsecp256k1.go (1)
chain/crypto/ethsecp256k1/keys.pb.go (6)
PrivKey(75-77)PrivKey(81-81)PrivKey(82-84)PubKey(30-32)PubKey(35-35)PubKey(36-38)
chain/exchange/types/authz_derivative.go (1)
chain/exchange/types/authz_common.go (1)
AuthorizedMarketsLimit(42-46)
chain/evm/types/tx.go (2)
chain/evm/types/utils.go (1)
DefaultPriorityReduction(24-24)chain/evm/types/errors.go (1)
ErrExecutionReverted(49-49)
chain/evm/types/events.go (1)
chain/evm/types/msg.go (1)
TypeMsgEthereumTx(40-40)
chain/exchange/types/common_order.go (1)
chain/exchange/types/exchange.pb.go (3)
OrderInfo(1185-1195)OrderInfo(1199-1199)OrderInfo(1200-1202)
chain/exchange/types/codec.go (1)
chain/exchange/types/authz.pb.go (3)
GenericExchangeAuthorization(615-621)GenericExchangeAuthorization(625-625)GenericExchangeAuthorization(626-628)
chain/exchange/types/fee_validation.go (1)
chain/exchange/types/exchange.pb.go (3)
FeeDiscountSchedule(2561-2572)FeeDiscountSchedule(2576-2576)FeeDiscountSchedule(2577-2579)
chain/exchange/types/params.go (2)
chain/exchange/types/paramset.go (1)
NewParamSetPair(16-18)chain/exchange/types/exchange.pb.go (5)
AtomicMarketOrderAccessLevel(31-31)AtomicMarketOrderAccessLevel(59-61)Params(232-314)Params(318-318)Params(319-321)
chain/exchange/types/errors.go (1)
chain/exchange/types/key.go (1)
ModuleName(17-17)
chain/evm/types/chain_config.go (1)
chain/evm/types/errors.go (1)
ErrInvalidChainConfig(55-55)
chain/evm/types/logs.go (2)
chain/types/validation.go (1)
IsEmptyHash(12-14)chain/evm/types/utils.go (1)
HexAddress(233-238)
chain/exchange/types/common_utils.go (6)
exchange/spot_exchange_rpc/pb/goadesign_goagen_injective_spot_exchange_rpc.pb.go (3)
Coin(4307-4316)Coin(4331-4331)Coin(4346-4348)exchange/auction_rpc/pb/goadesign_goagen_injective_auction_rpc.pb.go (3)
Coin(226-235)Coin(250-250)Coin(265-267)exchange/portfolio_rpc/pb/goadesign_goagen_injective_portfolio_rpc.pb.go (3)
Coin(379-388)Coin(403-403)Coin(418-420)exchange/campaign_rpc/pb/goadesign_goagen_injective_campaign_rpc.pb.go (3)
Coin(370-379)Coin(394-394)Coin(409-411)exchange/accounts_rpc/pb/goadesign_goagen_injective_accounts_rpc.pb.go (1)
Coin(1797-1806)exchange/explorer_rpc/pb/goadesign_goagen_injective_explorer_rpc.pb.go (3)
Coin(6252-6261)Coin(6276-6276)Coin(6291-6293)
chain/evm/types/access_list_tx.go (4)
chain/types/int.go (2)
SafeNewIntFromBigInt(43-48)IsValidInt256(59-61)chain/evm/types/access_list.go (2)
AccessList(10-10)NewAccessList(14-34)chain/evm/types/errors.go (1)
ErrInvalidGasFee(85-85)typeddata/typed_data.go (1)
ErrInvalidChainID(1285-1285)
chain/exchange/types/exchange.go (2)
chain/exchange/types/exchange.pb.go (15)
AtomicMarketOrderAccessLevel(31-31)AtomicMarketOrderAccessLevel(59-61)AtomicMarketOrderAccessLevel_Nobody(34-34)AtomicMarketOrderAccessLevel_SmartContractsOnly(37-37)AtomicMarketOrderAccessLevel_BeginBlockerSmartContractsOnly(36-36)AtomicMarketOrderAccessLevel_Everyone(38-38)DerivativeOrder(1449-1460)DerivativeOrder(1464-1464)DerivativeOrder(1465-1467)OrderInfo(1185-1195)OrderInfo(1199-1199)OrderInfo(1200-1202)SpotOrder(1251-1260)SpotOrder(1264-1264)SpotOrder(1265-1267)chain/exchange/types/common_utils.go (1)
Account(142-142)
chain/evm/types/params.go (1)
chain/evm/types/chain_config.go (1)
DefaultChainConfig(50-94)
chain/evm/types/dynamic_fee_tx.go (5)
chain/types/int.go (2)
SafeNewIntFromBigInt(43-48)IsValidInt256(59-61)chain/evm/types/access_list.go (2)
AccessList(10-10)NewAccessList(14-34)chain/evm/types/errors.go (2)
ErrInvalidGasCap(97-97)ErrInvalidGasFee(85-85)typeddata/typed_data.go (1)
ErrInvalidChainID(1285-1285)chain/evm/types/utils.go (1)
EffectiveGasPrice(228-230)
chain/evm/types/tx_data.go (4)
chain/evm/types/utils.go (1)
EffectiveGasPrice(228-230)chain/evm/types/dynamic_fee_tx.go (1)
NewDynamicFeeTx(16-59)chain/evm/types/access_list_tx.go (1)
NewAccessListTx(16-51)chain/evm/types/legacy_tx.go (1)
NewLegacyTx(12-42)
chain/evm/types/utils.go (3)
chain/evm/types/tx.pb.go (6)
MsgEthereumTxResponse(302-319)MsgEthereumTxResponse(323-323)MsgEthereumTxResponse(324-326)MsgEthereumTx(38-52)MsgEthereumTx(56-56)MsgEthereumTx(57-59)chain/evm/types/events.go (2)
EventTypeTxLog(7-7)AttributeKeyTxLog(16-16)chain/evm/types/logs.go (1)
LogsToEthereum(103-109)
🔇 Additional comments (64)
auth_vote/authz_vote.go (1)
20-20: Leverage default websocket endpoint by removing explicit path
This change simplifies the Tendermint RPC client setup by relying on the library’s built-in default (/websocket) instead of hard-coding it. Please verify that the default path incometbft/rpc/client/httpmatches your node’s configuration and doesn’t break any existing workflows.chain/erc20/types/codec.go (1)
35-42: Double-registration on Authz Amino codec may be unnecessaryYou already call
RegisterCodec(ModuleCdc)which registers all concrete types on the module’s Amino codec.
CallingRegisterCodec(authzcdc.Amino)immediately afterwards repeats the registration on the global Authz codec. If that’s explicitly required forauthz(e.g. to allowMsgExec.Msgspacking) keep it, otherwise it adds duplicate wiring and increases binary size.Please confirm the necessity or drop it to keep the init-path minimal.
chain/exchange/types/authz_spot.go (1)
44-44: Great concurrency improvement: Using thread-safe accessor for AuthorizedMarketsLimitThe change from directly accessing the
AuthorizedMarketsLimitconstant to using the thread-safe accessor functionAuthorizedMarketsLimit()improves concurrency safety across all authorization validation methods. This consistently applies the pattern established inauthz_common.gowhere a mutex protects access to the limit value.Also applies to: 84-84, 126-126, 166-166, 208-208
chain/crypto/ethsecp256k1/ethsecp256k1.go (1)
76-76: Code style improvement: Removed unused receiver namesRemoving the named receiver variables (
privKeyandpubKey) from theType()method declarations is a good stylistic improvement since these variables aren't used in the method bodies. This change makes the code more concise while maintaining the same functionality.Also applies to: 156-156
chain/exchange/types/authz_derivative.go (1)
44-44: Great concurrency improvement: Using thread-safe accessor for AuthorizedMarketsLimitThe change from directly accessing the
AuthorizedMarketsLimitconstant to using the thread-safe accessor functionAuthorizedMarketsLimit()improves concurrency safety across all derivative authorization validation methods. This consistently applies the same pattern used in the spot authorization types.Also applies to: 84-84, 126-126, 166-166, 208-208
chain/exchange/types/authz_common.go (5)
5-5: Appropriate package import for concurrency supportAdding the
syncpackage import is necessary to support the mutex-based concurrency control introduced in this file.
36-39: Good thread-safety implementation: Protecting shared state with mutexConverting
AuthorizedMarketsLimitfrom a constant to a protected variable with mutex synchronization is a good practice for state that may need to be modified at runtime or accessed concurrently.
41-53: Well-designed concurrency pattern: Thread-safe accessorsThe implementation of
AuthorizedMarketsLimit()andSetAuthorizedMarketsLimit(limit int)follows best practices for concurrent access:
- The getter uses a read lock which allows multiple concurrent readers
- The setter uses a write lock for exclusive access
- The getter uses
deferto ensure the lock is always releasedThis pattern ensures that the limit can be safely accessed and modified from multiple goroutines.
132-132: Consistent usage of thread-safe accessorUpdating the
ValidateBasic()method to use the newAuthorizedMarketsLimit()function instead of directly accessing the variable maintains consistency and ensures thread safety throughout the codebase.
120-120: Consider addressing the TODO commentThere's an unaddressed TODO comment about checking for "BO markets". Since this PR is adding Exchange V2 support, it might be a good opportunity to address this TODO if it's related to the V2 implementation.
Could you clarify if the TODO about checking "BO markets" is still relevant with Exchange V2, and if it should be addressed as part of this PR?
chain/crypto/hd/algorithm.go (4)
23-23: Documentation update to reflect Injective branding.The comment is now correctly referencing "Injective" instead of "Ethermint", aligning with the project's branding.
27-27: Documentation update to reflect Injective branding.Similar to the previous change, this comment now correctly references "Injective" instead of "Ethermint".
52-52: Stylistic improvement: Using unnamed receiver.Changed from named receiver to unnamed receiver since the receiver variable isn't referenced within the method body.
97-97: Stylistic improvement: Using unnamed receiver.Similar to the previous method, changed from named receiver to unnamed receiver for consistency.
chain/exchange/types/msgs_validation.go (2)
8-49: LGTM: Well-structured validation function with comprehensive checks.The new validation function for
UpdateSpotMarketMessagecorrectly:
- Validates the admin address format
- Ensures market ID is a valid hex hash
- Checks that at least one update field is provided
- Validates ticker length doesn't exceed the maximum allowed
- Properly validates tick sizes and minimum notional values when present
The error wrapping is clear and provides good context, using appropriate error codes from the errors module.
17-24: Good implementation of update validation logic.This code elegantly ensures that at least one update field is specified. Without this check, a message with no actual updates could be processed, leading to unnecessary transactions.
Makefile (7)
4-4: Updated dependency to newer version.Updating injective-indexer to v1.16.0-rc2 to support Exchange V2 functionality.
7-7: Updated dependency to newer version.Updating injective-core to v1.16.0-beta.2 to align with the Exchange V2 implementation.
54-75: Added ERC20 and EVM types.Expanded the copy-chain-types target to include the new ERC20 and EVM modules, which are necessary for Exchange V2 support. The EVM types particularly include comprehensive Ethereum transaction handling.
79-81: Added Exchange V2 types.Now copying the Exchange V2 types, which is central to the PR objective of adding Exchange V2 support.
137-138: Added Stream V2 types.Added support for Stream V2 protobuf files, necessary for the updated chain stream mentioned in the PR objectives.
142-144: Added additional utility types.Added int.go, util.go, and validation.go to the chain types, which likely provide supporting functionality for Exchange V2.
200-202: Added comprehensive linting target.The new lint-all target runs linting on the entire codebase without restricting to changed files, which is useful for ensuring overall code quality.
chain/exchange/types/codec.go (1)
169-170: Added registration for the new GenericExchangeAuthorization type.This addition registers the new
GenericExchangeAuthorizationtype as an implementation of theauthz.Authorizationinterface, enabling generic authorization for exchange operations.The implementation aligns with the PR objective of transitioning to Exchange V2 by providing more flexible authorization mechanisms.
chain/evm/types/tx.go (4)
11-29: Well-implemented transaction priority calculation with overflow protection.The
GetTxPriorityfunction correctly calculates Ethereum transaction priority with proper handling of big integers and safety checks to prevent overflow. The use ofDefaultPriorityReductionfollows standard practices for scaling gas prices to priorities.
31-34: Simple and effective transaction failure check.This implementation correctly determines if a transaction failed by checking the VM error string.
36-43: Good use of defensive copying to protect internal state.The
Returnmethod appropriately usescommon.CopyBytesto prevent mutation of internal state when returning transaction results, and correctly handles the failure case by returning nil.
45-52: Effective handling of revert data.The
Revertmethod properly extracts revert reason data only for REVERT operations, using defensive copying to protect internal state and correctly handling other error types.chain/exchange/types/params.go (4)
71-71: Good export of constant for wider use.Renaming
minMarginRatiotoMinMarginRatiomakes this constant available to other packages that might need to reference the minimum margin ratio value.
105-148: Standardized validation function naming in parameter set pairs.The consistent renaming of validation functions from
validateXxxtoValidateXxxin theParamSetPairsmethod improves API consistency and allows these validation functions to be used from other packages.
154-227: Standardized validation function calls in Validate method.All validation function calls in the
Validatemethod have been consistently updated to use the exported function names.
234-607: Comprehensive standardization of validation function exports.All parameter validation functions have been consistently renamed from unexported (
validateXxx) to exported (ValidateXxx) versions, maintaining the same validation logic while improving reusability across packages.chain/evm/types/access_list.go (3)
8-10: Clear definition of AccessList type for EIP-2930 compatibility.The AccessList type is properly defined as a slice of AccessTuple structs, enabling EIP-2930 access list functionality in the EVM module.
12-34: Well-implemented conversion from Ethereum core to protobuf-compatible AccessList.The
NewAccessListfunction correctly handles nil input, iterates through the Ethereum access list tuples, and performs appropriate conversions of addresses and storage keys to string format for protobuf compatibility.
36-55: Effective conversion from protobuf-compatible to Ethereum core AccessList.The
ToEthAccessListmethod properly converts the protobuf-compatible AccessList back to the Ethereum core format, handling address and storage key parsing appropriately.chain/exchange/types/common_order.go (4)
10-10: Updated to more efficient protobuf implementation.Switching from
github.com/golang/protobuf/prototogithub.com/gogo/protobuf/protois a good modernization as it provides a more performant implementation of Protocol Buffers.
15-30: Simplified binary options order margin calculation.Replacing the
OrderTypeparameter with a booleanisBuysimplifies the function and makes it more straightforward to use. The conditional logic is now clearer and more direct.
83-89: Added getter methods for order information.The new
GetPrice()andGetQuantity()methods provide a consistent interface for accessing order information, which helps maintain encapsulation principles even though the fields are already public.
122-122: Exported order hash computation function.Renaming
computeOrderHashtoComputeOrderHashmakes this utility function available to other packages, which improves reusability for testing and broader use cases.chain/exchange/types/errors.go (1)
115-116: Verify new error codes are wired-in and covered by tests
ErrInvalidExpirationBlock(107) andErrV1DerivativeMarketLaunch(108) are registered, but I could not find any usages in the diff.
• Make sure at least one code path returns each new error, otherwise the constants bloat the error table and future refactors may silently break.
• Add unit tests exercising the branches to lock the behaviour.If the usages live outside the shown diff, feel free to ignore.
chain/exchange/types/fee_validation.go (1)
10-15: No functional change – 👍
Signature re-formatting improves readability without altering logic.chain/crypto/hd/hd_path.go (1)
1-32: Well-structured implementation of HD path utilities for EthereumThe implementation provides clean utilities for working with hierarchical deterministic (HD) wallet paths, specifically supporting both default and Ledger Live derivation paths. The code is well-organized with proper error handling and clear documentation.
chain/exchange/types/key.go (1)
342-345: Good fix for handling empty string edge caseThe addition of the empty string check is a good improvement that handles the edge case when the string is empty after trimming, preventing potential parsing errors.
.golangci.yml (3)
1-4: Properly updated linter configuration versionThe update to configuration version "2" ensures compatibility with newer versions of golangci-lint and improved configuration structure.
19-47: Comprehensive linter settings configurationThe detailed configuration for gocritic and revive linters provides good customization of linting rules, with appropriate thresholds for line length and cognitive complexity.
47-54: Well-structured exclusion configurationThe exclusions section uses presets effectively to avoid common false positives and unnecessary noise in the linting output.
chain/evm/types/storage.go (4)
11-14: Good design choice for non-deterministic storage representationUsing a slice of State pairs instead of a map is a good approach to prevent non-determinism during genesis initialization and export operations, which is essential for blockchain state consistency.
16-30: Well-implemented validation logicThe validation logic correctly checks for duplicate keys and validates each state entry, providing clear error messages that include the index of the problematic state entry.
60-66: Good constructor implementationThe NewState constructor properly creates a new State from common.Hash values, converting them to strings for storage. This simplifies the creation of State objects and ensures consistent formatting.
43-48:✅ Verification successful
Consider implementing deep copy if State contains reference types
The current Copy method performs a shallow copy, which is appropriate for the current implementation since State doesn't contain reference types. However, if State is ever extended to include pointers or slices, this method should be updated to perform a deep copy.
Verify if the State type contains or will contain reference types:
🏁 Script executed:
#!/bin/bash # Check State struct definition to see if it contains reference types rg -A 10 "type State struct" chain/evm/types/Length of output: 805
Shallow copy is sufficient for current State struct
Verified that
chain/evm/types/state.pb.godefines State with only two string fields (Key and Value)—no pointers, slices, maps, or other reference types. TheCopymethod inchain/evm/types/storage.gotherefore correctly duplicates all data as-is. No changes needed.chain/evm/types/codec.go (1)
64-76: 🛠️ Refactor suggestionNil-safety missing in
PackTxDataCalling
PackTxData(nil)will dereference anilinterface and later panic when type-asserting. Add an explicitnilguard before the proto assertion.func PackTxData(txData TxData) (*codectypes.Any, error) { + if txData == nil { + return nil, errorsmod.Wrap(errortypes.ErrPackAny, "tx data cannot be nil") + } msg, ok := txData.(proto.Message)Likely an incorrect or invalid review comment.
chain/evm/types/logs.go (1)
55-69: Case-sensitive address validation may surprise users
Validatecallschaintypes.ValidateAddress, which usually enforces EIP-55 checksum casing. Down-stream tooling often emits lowercase addresses (e.g. geth when--nodiscover). Consider normalising to checksum or allowing lowercase to reduce friction.chain/exchange/types/authz_exchange_generic.go (1)
118-135:Acceptnever signals exhaustion of the spend limitAfter the loop,
a.SpendLimitmay be fully depleted, yet theAcceptResponsealways setsUpdated: &aand never togglesDelete.
Down-stream handlers will keep an “empty” authorization around, forcing every subsequent tx to execute the check even though no funds remain.- return authz.AcceptResponse{Accept: true, Updated: &a}, nil + if a.SpendLimit.IsZero() { + return authz.AcceptResponse{Accept: true, Delete: true}, nil + } + return authz.AcceptResponse{Accept: true, Updated: &a}, nilPlease confirm whether the SDK’s authz middleware already performs this clean-up automatically; if not, the patch above avoids stale grant entries.
chain/exchange/types/exchange.go (3)
32-41: Validation helper looks good
AtomicMarketOrderAccessLevel.IsValidexhaustively enumerates all protobuf variants and returns a sensible default. 👍
44-53: Unused structs may trigger dead-code/lint warnings
MatchedMarketDirectionandTradingRewardAccountPointswere re-introduced, but no code in the current diff references them. If they’re truly vestigial, consider deleting them; if not, add usage or unit tests to avoid future pruning by linters.
55-64:GetFillablenow aliasesGetQuantity– is that intended?Historically
fillableoften meant remaining quantity after partial fills. The new implementation just returns the original quantity, which could inflate matching logic elsewhere.-func (m *DerivativeOrder) GetFillable() math.LegacyDec { - return m.GetQuantity() -} +// If partial execution is possible, compute outstanding quantity instead. +func (m *DerivativeOrder) GetFillable() math.LegacyDec { + // TODO: subtract executed amount once that state is tracked. + return m.OrderInfo.Quantity +}chain/evm/types/access_list_tx.go (1)
183-224: Validation misses zero-value edge cases & re-uses expensive math checks
- A gas price of
0is technically allowed on Ethereum only for EIP-1559 base-fee burning; forAccessListTx(EIP-2930) a zero gas price would make the tx invalid on main-net. If the SDK mirrors that rule, add an explicitgasPrice.Sign() == 0check.IsValidInt256(tx.Fee())recomputes the fee big-int; this is inexpensive but can be saved by callingFee()once and re-using the value (you already need it for the overflow check).- Consider adding an upper bound check on
GasLimitto prevent DoS-style gigantic values.Not blocking, but tightening these guards improves safety.
chain/evm/types/legacy_tx.go (1)
159-192: Extend validation to cover gas-limit and zero gas-price corner casesAs with
AccessListTx, the validation logic could be hardened:
- Reject a
GasPriceof0– legacy transactions are invalid with zero gas price.- Enforce a sensible upper bound for
GasLimit(e.g.0 < gasLimit ≤ 30_000_000).- Confirm that
Fee()will not overflow 256-bits after multiplication.Adding these guards reduces the chance of malformed txs entering state.
chain/exchange/types/proposal.go (1)
792-794: 👍 Correct margin-ratio relation check.Requiring
initialMarginRatio > maintenanceMarginRatioprevents nonsensical or instantly-liquidatable markets. Looks good.Also applies to: 887-889
chain/exchange/types/msgs.go (4)
238-242: Margin ordering check-– LGTMSwitch to
<=guaranteesinitial > maintenance. Simple and correct.
345-396: Is zero‐margin intentionally allowed?
m.Margin.IsNil() || m.Margin.LT(0)now accepts0as valid. For non-reduce-only derivative orders, zero margin usually makes the order unfillable. Confirm business rule; if zero margin should be rejected change toLTE.
1395-1410: Good catch – correct sender / sub-account match logicComparing
sourceSubaccountIDagainstsenderAddrfixes previous false positives. ✔️
2013-2029: 🛠️ Refactor suggestionEdge case: duplicate detection treats “empty hash” as unique key
hasDuplicatesOrderstores everyhashandcidincluding the empty string.
Although a laterValidateBasicshould prevent both being empty, the duplicate-checking occurs before per-item validation inMsgBatchUpdateOrders.ValidateBasic. Two invalid records with both fields blank would thus be flagged as duplicates rather than “missing identifier”, which may hide the real error source.Proposal: run
hasDuplicatesOrderafter eachOrderData.ValidateBasicsucceeds, or ignore entries when bothhashandcidare empty.if hash != "" { if _, ok := seenHashes[hash]; ok { return true } seenHashes[hash] = struct{}{} } if cid != "" { if _, ok := seenCids[cid]; ok { return true } seenCids[cid] = struct{}{} }Likely an incorrect or invalid review comment.
chain/exchange/types/derivative_orders.go (1)
53-55: Inconsistent camel-case field name may hide bug
SubaccountId(lower-case “d”) is used here, whereas surrounding Go code often followsSubaccountID. If protobuf regenerates withSubaccountID, this accessor will compile but return the zero hash, silently breaking logic.Please double-check the generated struct tags and align the field name.
| type PositionPayout struct { | ||
| Payout math.LegacyDec | ||
| PnlNotional math.LegacyDec | ||
| IsProfitable bool | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Exported struct introduced without comment and all business logic removed
PositionPayoutis now exported but lacks a doc-comment → will fail lint.- All position-handling logic previously in this file has been deleted.
Unless that code was migrated elsewhere, callers referencing those helpers (ApplyFunding,ClosePosition, etc.) will fail to compile.
-type PositionPayout struct {
- Payout math.LegacyDec
- PnlNotional math.LegacyDec
- IsProfitable bool
-}
+// PositionPayout captures the settlement outcome for a derivative position.
+// TODO: update references after the V2 refactor.
+type PositionPayout struct {
+ Payout math.LegacyDec
+ PnlNotional math.LegacyDec
+ IsProfitable bool
+}Please verify that:
• All removed methods were intentionally relocated.
• No downstream packages still import them.
A quick compile-test will reveal breakages.
🏁 Script executed:
#!/bin/bash
set -e
echo "Looking for ApplyFunding definitions..."
rg "func ApplyFunding" -A2 || echo "❌ No ApplyFunding definition found"
echo "Looking for ClosePosition definitions..."
rg "func ClosePosition" -A2 || echo "❌ No ClosePosition definition found"
echo "Looking for calls to ApplyFunding..."
rg "ApplyFunding\(" -C2 || echo "❌ No ApplyFunding calls found"
echo "Looking for calls to ClosePosition..."
rg "ClosePosition\(" -C2 || echo "❌ No ClosePosition calls found"
echo "Looking for PositionPayout references across the repo..."
rg "PositionPayout" -C2 || echo "❌ No PositionPayout references outside this file"Length of output: 3162
Add a doc-comment for the exported PositionPayout
The PositionPayout struct in
chain/exchange/types/positions.go is now exported and consumed by the V2 code
(chain/exchange/types/v2/position.go → GetPayoutIfFullyClosing), but it lacks a
top-of-type comment (lint will error).
• File: chain/exchange/types/positions.go (around line 7)
• Suggested comment:
// PositionPayout captures the settlement outcome for a derivative position,
// including the realized payout amount, P&L notional, and whether it was profitable.All position-handling logic (ApplyFunding, ClosePosition, etc.) has been moved
to chain/exchange/types/v2/position.go, so no downstream breakages are expected.
🤖 Prompt for AI Agents
In chain/exchange/types/positions.go around lines 7 to 11, add a doc-comment
above the exported PositionPayout struct to satisfy lint requirements. Use the
suggested comment: "PositionPayout captures the settlement outcome for a
derivative position, including the realized payout amount, P&L notional, and
whether it was profitable." Confirm that all removed position-handling logic has
been relocated to chain/exchange/types/v2/position.go to avoid breaking
downstream code.
| uses: golangci/golangci-lint-action@v8 | ||
| with: | ||
| # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version | ||
| version: latest | ||
| version: v2.1.6 | ||
|
|
There was a problem hiding this comment.
version: v2.1.6 is invalid for golangci-lint
golangci-lint releases are still on the v1.x series (latest ≈ v1.55.x).
Setting version: v2.1.6 causes the action to 404 and the workflow to fail.
- version: v2.1.6
+# version: v1.55.2 # ← pin to an existing release
+ version: latest # or drop the line to track the defaultPlease adjust the version or omit the key entirely.
📝 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.
| uses: golangci/golangci-lint-action@v8 | |
| with: | |
| # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version | |
| version: latest | |
| version: v2.1.6 | |
| uses: golangci/golangci-lint-action@v8 | |
| with: | |
| # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version | |
| # version: v1.55.2 # ← pin to an existing release | |
| version: latest # or drop the line to track the default |
🤖 Prompt for AI Agents
In .github/workflows/golangci-lint.yml around lines 22 to 26, the golangci-lint
version is set to v2.1.6, which is invalid because golangci-lint releases are
still in the v1.x series. To fix this, change the version to a valid v1.x
version like v1.55.2 or remove the version key entirely to use the default
latest v1.x release.
| func UnpackTxData(any *codectypes.Any) (TxData, error) { | ||
| if any == nil { | ||
| return nil, errorsmod.Wrap(errortypes.ErrUnpackAny, "protobuf Any message cannot be nil") | ||
| } | ||
|
|
||
| txData, ok := any.GetCachedValue().(TxData) | ||
| if !ok { | ||
| return nil, errorsmod.Wrapf(errortypes.ErrUnpackAny, "cannot unpack Any into TxData %T", any) | ||
| } | ||
|
|
||
| return txData, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use UnpackAny helper to avoid empty cached values
any.GetCachedValue() returns nil unless the Any was previously unpacked, therefore the current check will falsely fail for freshly-received messages. Prefer codectypes.UnpackAny which fills the cache and performs type checking.
- txData, ok := any.GetCachedValue().(TxData)
- if !ok {
- return nil, errorsmod.Wrapf(errortypes.ErrUnpackAny, "cannot unpack Any into TxData %T", any)
- }
- return txData, nil
+ var txData TxData
+ if err := codectypes.UnpackAny(&txData, any); err != nil {
+ return nil, errorsmod.Wrap(errortypes.ErrUnpackAny, err.Error())
+ }
+ return txData, nil🤖 Prompt for AI Agents
In chain/evm/types/codec.go around lines 80 to 90, the code uses
any.GetCachedValue() which returns nil for freshly-received protobuf Any
messages, causing false unpack failures. Replace the use of any.GetCachedValue()
with the codectypes.UnpackAny helper function to properly unpack the Any
message, fill the cache, and perform type checking. Adjust the function to call
codectypes.UnpackAny with the Any message and a pointer to a TxData variable,
then return the unpacked TxData or an error accordingly.
| registry.RegisterImplementations( | ||
| (*sdk.Msg)(nil), | ||
| &MsgEthereumTx{}, | ||
| ) | ||
| registry.RegisterImplementations( | ||
| (*tx.TxExtensionOptionI)(nil), | ||
| &ExtensionOptionsEthereumTx{}, | ||
| ) | ||
| registry.RegisterImplementations( | ||
| (*sdk.Msg)(nil), | ||
| &MsgEthereumTx{}, | ||
| &MsgUpdateParams{}, | ||
| ) |
There was a problem hiding this comment.
Duplicate MsgEthereumTx registration can lead to runtime panics
MsgEthereumTx is registered twice for the same sdk.Msg interface (lines 37-40 and again at 46-49).
codectypes.InterfaceRegistry.RegisterImplementations panics on duplicate registrations, so this can crash the node at start-up.
- registry.RegisterImplementations(
- (*sdk.Msg)(nil),
- &MsgEthereumTx{},
- )
...
- registry.RegisterImplementations(
- (*sdk.Msg)(nil),
- &MsgEthereumTx{},
- &MsgUpdateParams{},
- )
+ registry.RegisterImplementations(
+ (*sdk.Msg)(nil),
+ &MsgEthereumTx{},
+ &MsgUpdateParams{},
+ )🤖 Prompt for AI Agents
In chain/evm/types/codec.go around lines 37 to 49, the MsgEthereumTx type is
registered twice for the sdk.Msg interface, which causes a runtime panic due to
duplicate registrations. To fix this, remove the redundant registration of
MsgEthereumTx in the second RegisterImplementations call and ensure each type is
registered only once per interface to prevent startup crashes.
| func ValidateHash(hex string) error { | ||
| if hex != "" && strings.TrimSpace(hex) == "" { | ||
| return errorsmod.Wrap(ErrInvalidChainConfig, "hash cannot be blank") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ValidateHash allows whitespace and malformed hashes
Currently the only invalid case is a non-empty all-whitespace string. This passes:
"EIP150Hash": "not-a-hex"Strengthen validation (see comment above) to protect against configuration mistakes.
🤖 Prompt for AI Agents
In chain/evm/types/chain_config.go around lines 178 to 184, the ValidateHash
function only rejects non-empty strings that are all whitespace but allows
malformed hashes like "not-a-hex". Update the function to check that the input
string is a valid hex-encoded hash of the expected length and format, returning
an error if it is not. This will prevent invalid or malformed hashes from
passing validation and protect against configuration mistakes.
| func (m *DerivativeMarketOrder) FillableQuantity() math.LegacyDec { | ||
| return m.OrderInfo.Quantity | ||
| } | ||
|
|
||
| func (m *DerivativeMarketOrder) Price() math.LegacyDec { | ||
| return m.OrderInfo.Price | ||
| } | ||
|
|
||
| func (m *DerivativeLimitOrder) Price() math.LegacyDec { | ||
| return m.OrderInfo.Price | ||
| } | ||
|
|
||
| func (m *DerivativeOrder) Price() math.LegacyDec { | ||
| return m.OrderInfo.Price | ||
| } | ||
|
|
||
| func (o *DerivativeOrder) IsConditional() bool { | ||
| return o.OrderType.IsConditional() | ||
| } | ||
|
|
||
| func (o *DerivativeMarketOrder) IsConditional() bool { | ||
| return o.OrderType.IsConditional() | ||
| } | ||
|
|
||
| func (o *DerivativeLimitOrder) IsConditional() bool { | ||
| return o.OrderType.IsConditional() | ||
| } | ||
|
|
||
| func (o *DerivativeLimitOrder) Cid() string { | ||
| return o.OrderInfo.GetCid() | ||
| } | ||
|
|
||
| func (o *DerivativeMarketOrder) Cid() string { | ||
| return o.OrderInfo.GetCid() | ||
| } | ||
|
|
||
| func (o *DerivativeOrder) SubaccountID() common.Hash { | ||
| return o.OrderInfo.SubaccountID() | ||
| } | ||
|
|
||
| func (o *DerivativeOrder) Cid() string { | ||
| return o.OrderInfo.GetCid() | ||
| } | ||
|
|
||
| func (o *DerivativeOrder) IsFromDefaultSubaccount() bool { | ||
| return o.OrderInfo.IsFromDefaultSubaccount() | ||
| } | ||
|
|
||
| func (o *DerivativeOrder) MarketID() common.Hash { | ||
| return common.HexToHash(o.MarketId) | ||
| } | ||
|
|
||
| func (o *DerivativeMarketOrder) SubaccountID() common.Hash { | ||
| return o.OrderInfo.SubaccountID() | ||
| } | ||
|
|
||
| func (o *DerivativeLimitOrder) SubaccountID() common.Hash { | ||
| return o.OrderInfo.SubaccountID() | ||
| } | ||
|
|
||
| func (o *OrderInfo) SubaccountID() common.Hash { | ||
| return common.HexToHash(o.SubaccountId) | ||
| } | ||
|
|
||
| func (o *OrderInfo) FeeRecipientAddress() common.Address { | ||
| address, _ := sdk.AccAddressFromBech32(o.FeeRecipient) | ||
| return common.BytesToAddress(address.Bytes()) | ||
| } | ||
|
|
||
| func (o *DerivativeLimitOrder) SdkAccAddress() sdk.AccAddress { | ||
| return sdk.AccAddress(o.SubaccountID().Bytes()[:common.AddressLength]) | ||
| } | ||
|
|
||
| func (o *DerivativeLimitOrder) IsFromDefaultSubaccount() bool { | ||
| return o.OrderInfo.IsFromDefaultSubaccount() | ||
| } | ||
|
|
||
| func (o *DerivativeMarketOrder) SdkAccAddress() sdk.AccAddress { | ||
| return sdk.AccAddress(o.SubaccountID().Bytes()[:common.AddressLength]) | ||
| } | ||
|
|
||
| func (o *DerivativeMarketOrder) IsFromDefaultSubaccount() bool { | ||
| return o.OrderInfo.IsFromDefaultSubaccount() | ||
| } | ||
|
|
||
| func (o *TrimmedDerivativeLimitOrder) IsReduceOnly() bool { | ||
| return o.Margin.IsZero() | ||
| } | ||
|
|
||
| func EmptyDerivativeMarketOrderResults() *DerivativeMarketOrderResults { | ||
| return &DerivativeMarketOrderResults{ | ||
| Quantity: math.LegacyZeroDec(), | ||
| Price: math.LegacyZeroDec(), | ||
| Fee: math.LegacyZeroDec(), | ||
| PositionDelta: PositionDelta{}, | ||
| Payout: math.LegacyZeroDec(), | ||
| } | ||
| o.SetQuantity(newQuantity) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
ResizeReduceOnlyOrder silently allows quantity increase
If newQuantity > oldQuantity, quantityDecrement is negative, the function returns nil, and the caller may proceed with an enlarged reduce-only order – violating the definition of “reduce only”.
Either:
- Explicitly reject increases:
if !quantityDecrement.IsPositive() {
- return nil
+ return ErrOrderInvalid.Wrap("new quantity must be smaller than current for reduce-only resize")
}- Or rename the helper to reflect that it is a no-op on increases.
Also consider adding an IsNegative() guard for newQuantity itself.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func ResizeReduceOnlyOrder(o IMutableDerivativeOrder, newQuantity math.LegacyDec) error { | |
| if o.IsVanilla() { | |
| return ErrOrderInvalid.Wrap("ResizeReduceOnlyOrder should only be used for reduce only orders!") | |
| } | |
| return computeOrderHash(marketId, o.OrderInfo.SubaccountId, o.OrderInfo.FeeRecipient, o.OrderInfo.Price.String(), o.OrderInfo.Quantity.String(), o.Margin.String(), triggerPrice, string(o.OrderType), nonce) | |
| } | |
| // ComputeOrderHash computes the order hash for given derivative limit order | |
| func (o *DerivativeOrder) ComputeOrderHash(nonce uint32) (common.Hash, error) { | |
| triggerPrice := "" | |
| if o.TriggerPrice != nil { | |
| triggerPrice = o.TriggerPrice.String() | |
| quantityDecrement := o.GetQuantity().Sub(newQuantity) | |
| if !quantityDecrement.IsPositive() { | |
| return nil | |
| } | |
| return computeOrderHash(o.MarketId, o.OrderInfo.SubaccountId, o.OrderInfo.FeeRecipient, o.OrderInfo.Price.String(), o.OrderInfo.Quantity.String(), o.Margin.String(), triggerPrice, string(o.OrderType), nonce) | |
| } | |
| func (o *DerivativeOrder) IsReduceOnly() bool { | |
| return o.Margin.IsZero() | |
| } | |
| func (o *DerivativeMarketOrder) IsReduceOnly() bool { | |
| return o.Margin.IsZero() | |
| } | |
| func (o *DerivativeLimitOrder) IsReduceOnly() bool { | |
| return o.Margin.IsZero() | |
| } | |
| func (o *DerivativeLimitOrder) Hash() common.Hash { | |
| return common.BytesToHash(o.OrderHash) | |
| } | |
| func (o *DerivativeMarketOrder) Hash() common.Hash { | |
| return common.BytesToHash(o.OrderHash) | |
| } | |
| func (o *DerivativeLimitOrder) FeeRecipient() common.Address { | |
| return o.OrderInfo.FeeRecipientAddress() | |
| } | |
| func (o *DerivativeMarketOrder) FeeRecipient() common.Address { | |
| return o.OrderInfo.FeeRecipientAddress() | |
| } | |
| func (o *DerivativeOrder) IsVanilla() bool { | |
| return !o.IsReduceOnly() | |
| } | |
| func (o *DerivativeMarketOrder) IsVanilla() bool { | |
| return !o.IsReduceOnly() | |
| } | |
| func (o *DerivativeLimitOrder) IsVanilla() bool { | |
| return !o.IsReduceOnly() | |
| } | |
| func (m *DerivativeMarketOrder) IsBuy() bool { | |
| return m.OrderType.IsBuy() | |
| } | |
| func (m *DerivativeLimitOrder) IsBuy() bool { | |
| return m.OrderType.IsBuy() | |
| } | |
| func (m *DerivativeOrder) IsBuy() bool { | |
| return m.OrderType.IsBuy() | |
| } | |
| func (m *DerivativeMarketOrder) Quantity() math.LegacyDec { | |
| return m.OrderInfo.Quantity | |
| } | |
| func (m *DerivativeMarketOrder) FillableQuantity() math.LegacyDec { | |
| return m.OrderInfo.Quantity | |
| } | |
| func (m *DerivativeMarketOrder) Price() math.LegacyDec { | |
| return m.OrderInfo.Price | |
| } | |
| func (m *DerivativeLimitOrder) Price() math.LegacyDec { | |
| return m.OrderInfo.Price | |
| } | |
| func (m *DerivativeOrder) Price() math.LegacyDec { | |
| return m.OrderInfo.Price | |
| } | |
| func (o *DerivativeOrder) IsConditional() bool { | |
| return o.OrderType.IsConditional() | |
| } | |
| func (o *DerivativeMarketOrder) IsConditional() bool { | |
| return o.OrderType.IsConditional() | |
| } | |
| func (o *DerivativeLimitOrder) IsConditional() bool { | |
| return o.OrderType.IsConditional() | |
| } | |
| func (o *DerivativeLimitOrder) Cid() string { | |
| return o.OrderInfo.GetCid() | |
| } | |
| func (o *DerivativeMarketOrder) Cid() string { | |
| return o.OrderInfo.GetCid() | |
| } | |
| func (o *DerivativeOrder) SubaccountID() common.Hash { | |
| return o.OrderInfo.SubaccountID() | |
| } | |
| func (o *DerivativeOrder) Cid() string { | |
| return o.OrderInfo.GetCid() | |
| } | |
| func (o *DerivativeOrder) IsFromDefaultSubaccount() bool { | |
| return o.OrderInfo.IsFromDefaultSubaccount() | |
| } | |
| func (o *DerivativeOrder) MarketID() common.Hash { | |
| return common.HexToHash(o.MarketId) | |
| } | |
| func (o *DerivativeMarketOrder) SubaccountID() common.Hash { | |
| return o.OrderInfo.SubaccountID() | |
| } | |
| func (o *DerivativeLimitOrder) SubaccountID() common.Hash { | |
| return o.OrderInfo.SubaccountID() | |
| } | |
| func (o *OrderInfo) SubaccountID() common.Hash { | |
| return common.HexToHash(o.SubaccountId) | |
| } | |
| func (o *OrderInfo) FeeRecipientAddress() common.Address { | |
| address, _ := sdk.AccAddressFromBech32(o.FeeRecipient) | |
| return common.BytesToAddress(address.Bytes()) | |
| } | |
| func (o *DerivativeLimitOrder) SdkAccAddress() sdk.AccAddress { | |
| return sdk.AccAddress(o.SubaccountID().Bytes()[:common.AddressLength]) | |
| } | |
| func (o *DerivativeLimitOrder) IsFromDefaultSubaccount() bool { | |
| return o.OrderInfo.IsFromDefaultSubaccount() | |
| } | |
| func (o *DerivativeMarketOrder) SdkAccAddress() sdk.AccAddress { | |
| return sdk.AccAddress(o.SubaccountID().Bytes()[:common.AddressLength]) | |
| } | |
| func (o *DerivativeMarketOrder) IsFromDefaultSubaccount() bool { | |
| return o.OrderInfo.IsFromDefaultSubaccount() | |
| } | |
| func (o *TrimmedDerivativeLimitOrder) IsReduceOnly() bool { | |
| return o.Margin.IsZero() | |
| } | |
| func EmptyDerivativeMarketOrderResults() *DerivativeMarketOrderResults { | |
| return &DerivativeMarketOrderResults{ | |
| Quantity: math.LegacyZeroDec(), | |
| Price: math.LegacyZeroDec(), | |
| Fee: math.LegacyZeroDec(), | |
| PositionDelta: PositionDelta{}, | |
| Payout: math.LegacyZeroDec(), | |
| } | |
| o.SetQuantity(newQuantity) | |
| return nil | |
| } | |
| func ResizeReduceOnlyOrder(o IMutableDerivativeOrder, newQuantity math.LegacyDec) error { | |
| if o.IsVanilla() { | |
| return ErrOrderInvalid.Wrap("ResizeReduceOnlyOrder should only be used for reduce only orders!") | |
| } | |
| quantityDecrement := o.GetQuantity().Sub(newQuantity) | |
| if !quantityDecrement.IsPositive() { | |
| return ErrOrderInvalid.Wrap("new quantity must be smaller than current for reduce-only resize") | |
| } | |
| o.SetQuantity(newQuantity) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In chain/exchange/types/derivative_orders.go around lines 160 to 172, the
ResizeReduceOnlyOrder function currently allows the newQuantity to be greater
than the old quantity, which violates the reduce-only order constraint. To fix
this, add a check that explicitly rejects any increase in quantity by returning
an error if newQuantity is greater than the current quantity. Additionally, add
a guard to reject negative newQuantity values by returning an error if
newQuantity is negative. This ensures the function enforces the reduce-only
semantics correctly.
| // Test Code Only (for v1 tests) | ||
| func (m *DerivativeOrder) CheckInitialMarginRequirementMarkPriceThreshold(initialMarginRatio, markPrice math.LegacyDec) (err error) { | ||
| // For Buys: MarkPrice ≥ (Margin - Price * Quantity) / ((InitialMarginRatio - 1) * Quantity) | ||
| // For Sells: MarkPrice ≤ (Margin + Price * Quantity) / ((1 + InitialMarginRatio) * Quantity) | ||
| markPriceThreshold := o.ComputeInitialMarginRequirementMarkPriceThreshold(initialMarginRatio) | ||
| return CheckInitialMarginMarkPriceRequirement(o.IsBuy(), markPriceThreshold, markPrice) | ||
| markPriceThreshold := m.ComputeInitialMarginRequirementMarkPriceThreshold(initialMarginRatio) | ||
| return CheckInitialMarginMarkPriceRequirement(m.OrderType.IsBuy(), markPriceThreshold, markPrice) | ||
| } | ||
|
|
||
| // Test Code Only (for v1 tests) | ||
| func CheckInitialMarginMarkPriceRequirement(isBuyOrLong bool, markPriceThreshold, markPrice math.LegacyDec) error { | ||
| if isBuyOrLong && markPrice.LT(markPriceThreshold) { | ||
| return errors.Wrapf(ErrInsufficientMargin, "Buy MarkPriceThreshold Check: mark/trigger price %s must be GTE %s", markPrice.String(), markPriceThreshold.String()) | ||
| return errors.Wrapf( | ||
| ErrInsufficientMargin, | ||
| "Buy MarkPriceThreshold Check: mark/trigger price %s must be GTE %s", markPrice.String(), markPriceThreshold.String()) | ||
| } else if !isBuyOrLong && markPrice.GT(markPriceThreshold) { | ||
| return errors.Wrapf(ErrInsufficientMargin, "Sell MarkPriceThreshold Check: mark/trigger price %s must be LTE %s", markPrice.String(), markPriceThreshold.String()) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // CheckValidConditionalPrice checks that conditional order type (STOP or TAKE) actually valid for current relation between triggerPrice and markPrice | ||
| func (o *DerivativeOrder) CheckValidConditionalPrice(markPrice math.LegacyDec) (err error) { | ||
| if !o.IsConditional() { | ||
| return nil | ||
| } | ||
|
|
||
| ok := true | ||
| switch o.OrderType { | ||
| case OrderType_STOP_BUY, OrderType_TAKE_SELL: // higher | ||
| ok = o.TriggerPrice.GT(markPrice) | ||
| case OrderType_STOP_SELL, OrderType_TAKE_BUY: // lower | ||
| ok = o.TriggerPrice.LT(markPrice) | ||
| } | ||
| if !ok { | ||
| return errors.Wrapf(ErrInvalidTriggerPrice, "order type %s incompatible with trigger price %s and markPrice %s", o.OrderType.String(), o.TriggerPrice.String(), markPrice.String()) | ||
| return errors.Wrapf( | ||
| ErrInsufficientMargin, | ||
| "Sell MarkPriceThreshold Check: mark/trigger price %s must be LTE %s", markPrice.String(), markPriceThreshold.String()) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Denominator can be zero in initial-margin threshold check
For buy orders the denominator is (InitialMarginRatio − 1) * Quantity.
If initialMarginRatio == 1 the code divides by zero, panicking inside math.LegacyDec. Guard early and surface an explanatory error.
if isBuyOrLong {
- denominator = initialMarginRatio.Sub(math.LegacyOneDec()).Mul(quantity)
+ diff := initialMarginRatio.Sub(math.LegacyOneDec())
+ if diff.IsZero() {
+ return math.LegacyZeroDec() // or return error up-stack
+ }
+ denominator = diff.Mul(quantity)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In chain/exchange/types/derivative_orders.go around lines 118 to 138, the
calculation for the initial margin threshold can cause a division by zero when
initialMarginRatio equals 1 for buy orders, as the denominator
(InitialMarginRatio - 1) * Quantity becomes zero. To fix this, add a guard
clause before performing the division in
ComputeInitialMarginRequirementMarkPriceThreshold or
CheckInitialMarginRequirementMarkPriceThreshold that checks if
initialMarginRatio equals 1 and return a clear error explaining the invalid
input instead of proceeding with the calculation.
| // Test Code Only (for v1 tests) | ||
| func (m *DerivativeOrder) ComputeOrderHash(nonce, scaleFactor uint32) (common.Hash, error) { | ||
| triggerPrice := "" | ||
| if m.TriggerPrice != nil { | ||
| triggerPrice = m.TriggerPrice.String() | ||
| } | ||
| // Margin = (scaled(1) - Price) * Quantity for sells | ||
| return o.OrderInfo.Quantity.Mul(GetScaledPrice(math.LegacyOneDec(), oracleScaleFactor).Sub(o.Price())) | ||
| } | ||
|
|
||
| func (o *DerivativeMarketOrder) GetRequiredBinaryOptionsMargin(oracleScaleFactor uint32) math.LegacyDec { | ||
| // Margin = Price * Quantity for buys | ||
| if o.IsBuy() { | ||
| notional := o.Price().Mul(o.OrderInfo.Quantity) | ||
| return notional | ||
| } | ||
| // Margin = (scaled(1) - Price) * Quantity for sells | ||
| return o.OrderInfo.Quantity.Mul(GetScaledPrice(math.LegacyOneDec(), oracleScaleFactor).Sub(o.Price())) | ||
| m.OrderInfo.Price = PriceFromChainFormat(m.OrderInfo.Price, 0, scaleFactor) | ||
| m.Margin = NotionalFromChainFormat(m.Margin, scaleFactor) | ||
|
|
||
| return ComputeOrderHash( | ||
| m.MarketId, | ||
| m.OrderInfo.SubaccountId, | ||
| m.OrderInfo.FeeRecipient, | ||
| m.OrderInfo.Price.String(), | ||
| m.OrderInfo.Quantity.String(), | ||
| m.Margin.String(), | ||
| triggerPrice, | ||
| string(m.OrderType), | ||
| nonce) | ||
| } |
There was a problem hiding this comment.
DerivativeOrder.ComputeOrderHash mutates receiver – surprising side-effect
The method rewrites m.OrderInfo.Price and m.Margin via PriceFromChainFormat / NotionalFromChainFormat. When test code calls the method twice, the 2nd hash will be different, and callers might not expect the order object to be converted in-place.
If mutation is unnecessary, make defensive copies:
-price := PriceFromChainFormat(m.OrderInfo.Price, 0, scaleFactor)
-margin := NotionalFromChainFormat(m.Margin, scaleFactor)
-
-m.OrderInfo.Price = price
-m.Margin = margin
+price := PriceFromChainFormat(m.OrderInfo.Price, 0, scaleFactor)
+margin := NotionalFromChainFormat(m.Margin, scaleFactor)and pass the local variables to ComputeOrderHash.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In chain/exchange/types/derivative_orders.go lines 77 to 97, the
ComputeOrderHash method mutates the receiver by modifying m.OrderInfo.Price and
m.Margin in place, causing unexpected side effects on repeated calls. To fix
this, avoid mutating the receiver by creating local copies of Price and Margin
converted via PriceFromChainFormat and NotionalFromChainFormat, then pass these
local variables to ComputeOrderHash instead of modifying the fields on m
directly.
| // Test Code Only (for v1 tests) | ||
| func (m *DerivativeLimitOrder) GetCancelRefundAmount(feeRate math.LegacyDec) math.LegacyDec { | ||
| marginHoldRefund := math.LegacyZeroDec() | ||
| if o.IsVanilla() { | ||
| // negative fees are only accounted for upon matching | ||
| if !m.Margin.IsZero() { | ||
| positiveFeePart := math.LegacyMaxDec(math.LegacyZeroDec(), feeRate) | ||
| //nolint:all | ||
| // Refund = (FillableQuantity / Quantity) * (Margin + Price * Quantity * feeRate) | ||
| notional := o.OrderInfo.Price.Mul(o.OrderInfo.Quantity) | ||
| marginHoldRefund = o.Fillable.Mul(o.Margin.Add(notional.Mul(positiveFeePart))).Quo(o.OrderInfo.Quantity) | ||
| notional := m.OrderInfo.Price.Mul(m.OrderInfo.Quantity) | ||
| marginHoldRefund = m.Fillable.Mul(m.Margin.Add(notional.Mul(positiveFeePart))).Quo(m.OrderInfo.Quantity) | ||
| } | ||
| return marginHoldRefund | ||
| } |
There was a problem hiding this comment.
Division-by-zero possibility when Quantity is zero
GetCancelRefundAmount divides by m.OrderInfo.Quantity without guarding against a zero quantity limit-order. A malformed test fixture will panic.
+if m.OrderInfo.Quantity.IsZero() {
+ return math.LegacyZeroDec()
+}
marginHoldRefund = m.Fillable.Mul(...).Quo(m.OrderInfo.Quantity)📝 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.
| // Test Code Only (for v1 tests) | |
| func (m *DerivativeLimitOrder) GetCancelRefundAmount(feeRate math.LegacyDec) math.LegacyDec { | |
| marginHoldRefund := math.LegacyZeroDec() | |
| if o.IsVanilla() { | |
| // negative fees are only accounted for upon matching | |
| if !m.Margin.IsZero() { | |
| positiveFeePart := math.LegacyMaxDec(math.LegacyZeroDec(), feeRate) | |
| //nolint:all | |
| // Refund = (FillableQuantity / Quantity) * (Margin + Price * Quantity * feeRate) | |
| notional := o.OrderInfo.Price.Mul(o.OrderInfo.Quantity) | |
| marginHoldRefund = o.Fillable.Mul(o.Margin.Add(notional.Mul(positiveFeePart))).Quo(o.OrderInfo.Quantity) | |
| notional := m.OrderInfo.Price.Mul(m.OrderInfo.Quantity) | |
| marginHoldRefund = m.Fillable.Mul(m.Margin.Add(notional.Mul(positiveFeePart))).Quo(m.OrderInfo.Quantity) | |
| } | |
| return marginHoldRefund | |
| } | |
| // Test Code Only (for v1 tests) | |
| func (m *DerivativeLimitOrder) GetCancelRefundAmount(feeRate math.LegacyDec) math.LegacyDec { | |
| // Guard against division by zero | |
| if m.OrderInfo.Quantity.IsZero() { | |
| return math.LegacyZeroDec() | |
| } | |
| marginHoldRefund := math.LegacyZeroDec() | |
| if !m.Margin.IsZero() { | |
| positiveFeePart := math.LegacyMaxDec(math.LegacyZeroDec(), feeRate) | |
| notional := m.OrderInfo.Price.Mul(m.OrderInfo.Quantity) | |
| marginHoldRefund = m.Fillable. | |
| Mul(m.Margin.Add(notional.Mul(positiveFeePart))). | |
| Quo(m.OrderInfo.Quantity) | |
| } | |
| return marginHoldRefund | |
| } |
🤖 Prompt for AI Agents
In chain/exchange/types/derivative_orders.go around lines 107 to 116, the method
GetCancelRefundAmount divides by m.OrderInfo.Quantity without checking if it is
zero, which can cause a division-by-zero panic. Add a guard clause to return
zero or handle the case when m.OrderInfo.Quantity is zero before performing the
division to prevent runtime panics.
| func GetScaledPrice(price math.LegacyDec, scaleFactor uint32) math.LegacyDec { | ||
| return price.Mul(math.LegacyNewDec(10).Power(uint64(scaleFactor))) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Overflow / precision-loss risk in GetScaledPrice
Scaling the price by 10^scaleFactor inside decimal arithmetic can silently overflow the math.LegacyDec internal sdk.Int if callers pass an unusually large scaleFactor (e.g. user-provided or un-validated values). A defensive upper-bound check (or at least a documented expectation) would prevent hard-to-trace panics.
func GetScaledPrice(price math.LegacyDec, scaleFactor uint32) math.LegacyDec {
- return price.Mul(math.LegacyNewDec(10).Power(uint64(scaleFactor)))
+ if scaleFactor > 18 { // 10^18 ≈ 1e18 keeps us within 256-bit Int range
+ panic("scaleFactor too large – would overflow")
+ }
+ return price.Mul(math.LegacyNewDec(10).Power(uint64(scaleFactor)))
}This will help us verify how Power is implemented and whether overflow checks exist.
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Inspect chain/exchange/types/derivative_orders.go ==="
grep -R "GetScaledPrice" -n chain/exchange/types/derivative_orders.go || true
sed -n '1,50p' chain/exchange/types/derivative_orders.go || true
echo
echo "=== Locate LegacyNewDec definition ==="
file=$(rg -l "LegacyNewDec" -n . | head -n1)
echo "File: $file"
sed -n '1,200p' "$file" || true
echo
echo "=== Inspect Power method in the same file ==="
grep -R "Power(" -n "$file" || true
sed -n '200,300p' "$file" || trueLength of output: 10522
Unbounded exponent in GetScaledPrice may cause overflow or excessive computation
chain/exchange/types/derivative_orders.go:9
GetScaledPrice does
return price.Mul(math.LegacyNewDec(10).Power(uint64(scaleFactor)))without validating scaleFactor. Extremely large exponents can produce very large underlying integers (or overflow if the sdk.Int implementation is bounded), leading to panics or severe performance degradation. Please verify that callers cannot supply untrusted or unbounded values, and consider one of:
- Adding an explicit upper‐bound check (e.g.
if scaleFactor > 18 { panic(...) }), or - Documenting the maximum allowed
scaleFactor(e.g. “must be ≤18”) for all callers.
🤖 Prompt for AI Agents
In chain/exchange/types/derivative_orders.go around lines 9 to 11, the
GetScaledPrice function multiplies price by 10 raised to scaleFactor without
validating scaleFactor, risking overflow or excessive computation. To fix this,
add an explicit upper-bound check on scaleFactor (for example, panic or return
an error if scaleFactor exceeds 18) before performing the power operation, or
alternatively, document clearly that scaleFactor must not exceed a safe maximum
like 18 to ensure callers do not pass untrusted or unbounded values.
Solves CP-127
Summary by CodeRabbit
New Features
Improvements
Removals
Bug Fixes
Chores