Skip to content

v1.58.x with EVM support #305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: feat/add_exchange_v2_support
Choose a base branch
from

Conversation

maxim-inj
Copy link
Collaborator

@maxim-inj maxim-inj commented May 5, 2025

Addressing some concerns from previous PR on the same theme:

  1. The types code copied from https://github.com/InjectiveLabs/injective-core/tree/feat-evm-v50 branch
  2. We have to make a tag on the sdk-go, because it's used inside internal processes, i.e. testing, etc. Way before official release is confirmed.
  3. The code has to be copied by maintainers of the original code because in this case it was more complicated than just copying files around. The process could be improved, yeah.
  4. Makefile was updated with best-effort actions, it won't work out of the box on the master branch of injective-core, but I think it's a shortsighted approach anyway.

We'll improve the files on the core side to the point where they could be easily copied around. For now we have to stick to manual copy with some overrides.

@maxim-inj maxim-inj requested a review from aarmoa May 5, 2025 08:13
@maxim-inj maxim-inj self-assigned this May 5, 2025
Copy link

coderabbitai bot commented May 5, 2025

Walkthrough

This update introduces extensive changes across the codebase, with a primary focus on expanding EVM (Ethereum Virtual Machine) and ERC20 module support, simplifying and refactoring exchange module types, and improving code organization and validation. The EVM module receives a comprehensive set of new files implementing transaction types, error handling, parameter management, utilities, and contract embedding. The ERC20 module gains new type definitions, message handling, error registration, and parameter scaffolding. The exchange module undergoes significant simplification: many methods and types related to order management, market status, positions, and proposals are removed or refactored in favor of streamlined interfaces and utility functions. Additional improvements include new key prefixes for order expirations, enhanced validation routines, and updates to the Makefile for broader dependency coverage.

Changes

File(s) Change Summary
Makefile Updated to clone a specific branch of injective-core and expanded the copy-chain-types target to include new EVM, ERC20, exchange v2, and stream v2 types.
chain/crypto/ethsecp256k1/ethsecp256k1.go, chain/crypto/hd/algorithm.go Method receivers changed from named to unnamed for consistency; no logic changes.
chain/erc20/types/* New files: codec.go, errors.go, msgs.go, params.go, types.go define codec, error registration, message types, parameter scaffolding, and denom validation for the ERC20 module.
chain/evm/types/* Major addition: new files implement EVM transaction types (legacy, access list, dynamic fee), error handling, parameter management, contract embedding, key management, logs, utilities, and validation. JSON contract artifacts for ERC20 and test contracts are embedded.
chain/exchange/types/common_order.go Simplified margin calculation logic, added price/quantity getters, exported order hash computation.
chain/exchange/types/common_utils.go Removed order delta structs and subaccount ID method; added generic duplicate checkers.
chain/exchange/types/deposit.go Removed deposit constructor, emptiness check, and display method.
chain/exchange/types/derivative_orders.go Removed complex margin, hash, and conversion logic; added basic accessors, simplified hash computation, and test-only margin checks.
chain/exchange/types/errors.go Improved error messages, added new error codes for expiration and market launch.
chain/exchange/types/exchange.go Removed most order and market status methods; consolidated to minimal interface; added access level validation.
chain/exchange/types/fee_discounts.go Removed all fee discount schedule methods and constructors, leaving only the struct/type alias.
chain/exchange/types/fee_validation.go Reformatted function signatures for clarity; added test-only comment.
chain/exchange/types/genesis.go Removed default and new genesis state constructors.
chain/exchange/types/key.go Added prefixes and functions for order expiration keys.
chain/exchange/types/market.go Removed all market struct methods; added stateless conversion functions for price/quantity/notional.
chain/exchange/types/market_admin.go Removed permission-checking methods, fixed permission constant typos, added composite and new permission constants.
chain/exchange/types/msgs.go Introduced UpdateSpotMarketMessage interface, standardized validation, added binary options order constructor, improved duplicate detection.
chain/exchange/types/msgs_validation.go Added shared validation function for spot market update messages.
chain/exchange/types/params.go Renamed validation functions for consistency, removed param constructors, exported min margin ratio.
chain/exchange/types/positions.go Removed all position logic except for an exported payout struct.
chain/exchange/types/proposal.go Removed all proposal constructors; tightened margin validation in proposals.
chain/exchange/types/spot_orders.go Removed all methods from spot order types; introduced IOrderInfo interface and standalone order hash function.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CosmosSDK
    participant EVMTypes
    participant ERC20Types
    participant ExchangeTypes

    User->>CosmosSDK: Submit transaction (EVM/ERC20/Exchange)
    CosmosSDK->>EVMTypes: If EVM tx, decode and validate
    CosmosSDK->>ERC20Types: If ERC20 tx, decode and validate
    CosmosSDK->>ExchangeTypes: If Exchange tx, decode and validate
    EVMTypes->>CosmosSDK: Return validation result
    ERC20Types->>CosmosSDK: Return validation result
    ExchangeTypes->>CosmosSDK: Return validation result
    CosmosSDK->>User: Accept or reject transaction
Loading

Poem

🐇
New types and tools, a codebase reborn,
EVM and ERC20 modules adorn.
Exchange refactored, less clutter in sight—
Orders and markets now lean and light.
With hashes and keys, and contracts that gleam,
The rabbit hops on in a blockchain dream!

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@maxim-inj maxim-inj changed the base branch from dev to feat/add_exchange_v2_support May 5, 2025 08:14
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 29

🔭 Outside diff range comments (3)
chain/exchange/types/fee_validation.go (1)

46-50: ⚠️ Potential issue

Robustly derive the maximum taker discount

ValidateMakerWithTakerFeeAndDiscounts assumes the last element of TierInfos carries the highest TakerDiscountRate.
If the slice is ever unsorted (e.g. constructed in tests), the smallest taker-fee rate can be mis-computed.

-	maxTakerDiscount := discountSchedule.TierInfos[len(discountSchedule.TierInfos)-1].TakerDiscountRate
-	smallestTakerFeeRate = smallestTakerFeeRate.Mul(math.LegacyOneDec().Sub(maxTakerDiscount))
+	// Find the highest discount defensively.
+	maxTakerDiscount := discountSchedule.TierInfos[0].TakerDiscountRate
+	for _, tier := range discountSchedule.TierInfos[1:] {
+		if tier.TakerDiscountRate.GT(maxTakerDiscount) {
+			maxTakerDiscount = tier.TakerDiscountRate
+		}
+	}
+	smallestTakerFeeRate = smallestTakerFeeRate.Mul(math.LegacyOneDec().Sub(maxTakerDiscount))
chain/exchange/types/params.go (1)

589-597: ⚠️ Potential issue

Fix swapped placeholders in error message

ValidateAtomicMarketOrderFeeMultiplier prints the current value where the limit should appear and vice-versa, confusing operators.

-	return fmt.Errorf("atomicMarketOrderFeeMultiplier cannot be bigger than %v: %v", v, MaxFeeMultiplier)
+	return fmt.Errorf("atomicMarketOrderFeeMultiplier cannot be bigger than %v: %v", MaxFeeMultiplier, v)
chain/exchange/types/spot_orders.go (1)

53-60: ⚠️ Potential issue

Return value on error has wrong type – compilation will fail

ComputeSpotOrderHash returns (common.Hash, error) but in error branches it returns AuctionSubaccountID (likely a string). Replace with zero hash:

-    return AuctionSubaccountID, err
+    return common.Hash{}, err

Do this for both error paths (domain & message hash).

🧹 Nitpick comments (38)
chain/exchange/types/params.go (1)

71-71: Exported var looks like a constant – document intent

MinMarginRatio is now exported but mutable (var).
Accidental re-assignment from other packages would silently relax risk limits.
Consider exposing an accessor or turning it into an unexported var with an exported getter.

chain/evm/types/ERC20Contract.json (1)

5-5: Minor: Remove trailing newline after JSON object

There's an unnecessary trailing newline at line 5 after the JSON object's closing brace.

 }
-
chain/erc20/types/errors.go (1)

9-10: Incorrect module name in comment

The comment refers to "x/tokenfactory module sentinel errors" but these errors are registered with the "erc20" module name.

-// x/tokenfactory module sentinel errors
+// x/erc20 module sentinel errors
chain/evm/types/validation.go (4)

11-14: Implementation looks good, but consider handling invalid hex input.

The function correctly checks for empty hashes by comparing bytes, but it doesn't handle the case where the input string is not a valid hex string. The common.HexToHash function will convert any string to a hash, potentially masking validation errors.

// IsEmptyHash returns true if the hash corresponds to an empty ethereum hex hash.
func IsEmptyHash(hash string) bool {
+   // Check if the string is a valid hex string first
+   if len(hash) > 0 && (len(hash) < 2 || hash[:2] != "0x" || !isHex(hash[2:])) {
+       return false
+   }
    return bytes.Equal(common.HexToHash(hash).Bytes(), common.Hash{}.Bytes())
}

+// isHex returns true if the string consists of only hex characters.
+func isHex(s string) bool {
+   for _, c := range s {
+       if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F')) {
+           return false
+       }
+   }
+   return true
+}

16-19: Similar to IsEmptyHash, consider handling invalid hex input.

The function performs the correct comparison for checking zero addresses but doesn't validate that the input is a valid hex string before conversion.

// IsZeroAddress returns true if the address corresponds to an empty ethereum hex address.
func IsZeroAddress(address string) bool {
+   // Validate the address is a valid hex address first
+   if !common.IsHexAddress(address) {
+       return false
+   }
    return bytes.Equal(common.HexToAddress(address).Bytes(), common.Address{}.Bytes())
}

21-30: The function documentation is incomplete.

The comment for ValidateAddress is incomplete. It states "either not a hex formatted string address" but doesn't complete the sentence explaining what other conditions might cause an error.

-// ValidateAddress returns an error if the provided string is either not a hex formatted string address
+// ValidateAddress returns an error if the provided string is not a hex formatted Ethereum address
func ValidateAddress(address string) error {
    if !common.IsHexAddress(address) {
        return errorsmod.Wrapf(
            errortypes.ErrInvalidAddress, "address '%s' is not a valid ethereum hex address",
            address,
        )
    }
    return nil
}

32-42: Optimize ValidateNonZeroAddress for better performance.

The current implementation calls ValidateAddress after checking if the address is zero, which results in potential duplicate validation. Since IsZeroAddress internally uses common.HexToAddress, we could optimize by first validating the address format and then checking if it's zero.

// ValidateNonZeroAddress returns an error if the provided string is not a hex
// formatted string address or is equal to zero
func ValidateNonZeroAddress(address string) error {
+   if err := ValidateAddress(address); err != nil {
+       return err
+   }
    if IsZeroAddress(address) {
        return errorsmod.Wrapf(
            errortypes.ErrInvalidAddress, "address '%s' must not be zero",
            address,
        )
    }
-   return ValidateAddress(address)
+   return nil
}
chain/evm/types/events.go (1)

3-24: Add documentation for event constants to improve code clarity.

The constants are well-named but most lack comments explaining their specific purpose, which would improve maintainability and onboarding for new developers.

// Evm module events
const (
    EventTypeEthereumTx = TypeMsgEthereumTx
+   // EventTypeBlockBloom represents a block bloom filter event
    EventTypeBlockBloom = "block_bloom"
+   // EventTypeTxLog represents an Ethereum transaction log event
    EventTypeTxLog      = "tx_log"

+   // AttributeKeyContractAddress is the attribute key for the contract address
    AttributeKeyContractAddress = "contract"
+   // AttributeKeyRecipient is the attribute key for the transaction recipient
    AttributeKeyRecipient       = "recipient"
+   // AttributeKeyTxHash is the attribute key for the transaction hash
    AttributeKeyTxHash          = "txHash"
+   // AttributeKeyEthereumTxHash is the attribute key for the Ethereum transaction hash
    AttributeKeyEthereumTxHash  = "ethereumTxHash"
+   // AttributeKeyTxIndex is the attribute key for the transaction index
    AttributeKeyTxIndex         = "txIndex"
+   // AttributeKeyTxGasUsed is the attribute key for the gas used by the transaction
    AttributeKeyTxGasUsed       = "txGasUsed"
+   // AttributeKeyTxType is the attribute key for the transaction type
    AttributeKeyTxType          = "txType"
+   // AttributeKeyTxLog is the attribute key for the transaction log
    AttributeKeyTxLog           = "txLog"
    // tx failed in eth vm execution
    AttributeKeyEthereumTxFailed = "ethereumTxFailed"
+   // AttributeValueCategory defines the module name for event attributes
    AttributeValueCategory       = ModuleName
+   // AttributeKeyEthereumBloom is the attribute key for the ethereum bloom filter
    AttributeKeyEthereumBloom    = "bloom"

+   // MetricKeyTransitionDB is the metric key for database transitions
    MetricKeyTransitionDB = "transition_db"
+   // MetricKeyStaticCall is the metric key for static calls
    MetricKeyStaticCall   = "static_call"
)
chain/evm/types/storage.go (2)

32-40: Consider handling empty storage more explicitly in the String method.

While functionally correct, the String method could be improved by explicitly handling empty Storage slices.

// String implements the stringer interface
func (s Storage) String() string {
+   if len(s) == 0 {
+       return "empty storage"
+   }
    var str string
    for _, state := range s {
        str += fmt.Sprintf("%s\n", state.String())
    }

    return str
}

42-48: Ensure Copy method performs a true deep copy if needed.

The current implementation performs a shallow copy which works for immutable State structs with string fields, but would not be sufficient if State contained mutable fields or pointers.

// Copy returns a copy of storage.
func (s Storage) Copy() Storage {
    cpy := make(Storage, len(s))
-   copy(cpy, s)
+   for i, state := range s {
+       // Create a new State with the same values
+       cpy[i] = State{
+           Key:   state.Key,
+           Value: state.Value,
+       }
+   }

    return cpy
}
chain/erc20/types/codec.go (1)

9-9: Remove Starport scaffolding comment.

The comment "this line is used by starport scaffolding # 1" appears to be an artifact from code generation and should be removed.

-	// this line is used by starport scaffolding # 1
	"github.com/cosmos/cosmos-sdk/types/msgservice"
chain/exchange/types/key.go (2)

121-124: New prefixes look consistent, but please double-check for collisions

0x85 and 0x86 are unused elsewhere in this file, so the two new prefixes should be safe.
Still, consider adding a quick unit-test that iterates all prefix constants and fails on duplicates – this guards future edits.


565-585: Minor optimisation & naming consistency for GetOrderExpirationMarketPrefix

  1. make(..., len(OrderExpirationsPrefix)+len(blockNumberBz)) allocates a slice that is one byte shorter than needed, because the function actually appends OrderExpirationMarketsPrefix (note the extra “s”).
  2. Capacity mis-size is harmless at runtime, but it forces an extra allocation when append grows the slice.
  3. Function name is singular (Market) whereas the constant is plural (Markets). Aligning them avoids confusion.

Suggested diff:

-func GetOrderExpirationMarketPrefix(blockNumber int64) []byte {
+func GetOrderExpirationMarketPrefix(blockNumber int64) []byte {
     blockNumberBz := sdk.Uint64ToBigEndian(uint64(blockNumber))

-    buf := make([]byte, 0, len(OrderExpirationsPrefix)+len(blockNumberBz))
-    buf = append(buf, OrderExpirationMarketsPrefix...)
+    buf := make([]byte, 0, len(OrderExpirationMarketsPrefix)+len(blockNumberBz))
+    buf = append(buf, OrderExpirationMarketsPrefix...)
     buf = append(buf, blockNumberBz...)

     return buf
 }
chain/exchange/types/common_utils.go (1)

311-342: Duplicate-detection helpers – tighten edge-cases

  1. HasDuplicatesHexHash silently treats invalid hex strings as the zero hash, so a single bad value may produce many false “duplicates”.
    Guard the conversion with IsHexHash(item) first, or return an error to the caller.

  2. HasDuplicatesCoin compares only Denom. Consider normalising to lower-case to avoid "USDC" vs "usdc" mismatches:

-       if _, ok := seen[item.Denom]; ok {
+       denom := strings.ToLower(item.Denom)
+       if _, ok := seen[denom]; ok {
            return true
        }
-       seen[item.Denom] = struct{}{}
+       seen[denom] = struct{}{}

Neither change is strictly required for correctness today, but both make the helpers safer for future inputs.

chain/evm/types/codec.go (3)

35-57: Duplicate sdk.Msg registrations – safe but redundant

MsgEthereumTx is registered as an sdk.Msg twice (lines 37-41 and 46-50).
The interface registry tolerates this, yet keeping a single block is clearer:

-   registry.RegisterImplementations(
-       (*sdk.Msg)(nil),
-       &MsgEthereumTx{},
-   )
-   registry.RegisterImplementations(
-       (*tx.TxExtensionOptionI)(nil),
-       &ExtensionOptionsEthereumTx{},
-   )
-   registry.RegisterImplementations(
+   registry.RegisterImplementations(
        (*sdk.Msg)(nil),
        &MsgEthereumTx{},
+       &MsgUpdateParams{},
    )
+   registry.RegisterImplementations(
+       (*tx.TxExtensionOptionI)(nil),
+       &ExtensionOptionsEthereumTx{},
+   )

61-76: PackTxData: defer to proto-registry for smaller code

Current implementation relies on the concrete type already being linked in and castable to proto.Message.
That is fine, but you can shrink the helper by delegating to codectypes.NewAnyWithValue directly and letting it perform the cast:

func PackTxData(txData TxData) (*codectypes.Any, error) {
    return codectypes.NewAnyWithValue(txData)
}

No functional difference, just less code.


80-90: UnpackTxData assumes cached value

any.GetCachedValue() is only populated when the proto Any has been unpacked once before; after a fresh unmarshal it is nil.
Safer pattern:

var txData TxData
if err := ModuleCdc.UnpackAny(any, &txData); err != nil {
    return nil, err
}
return txData, nil

This guarantees correct behaviour even when the cache is empty.

chain/evm/types/compiled_contract.go (2)

17-19: Use hex.EncodeToString instead of fmt.Sprintf for cleaner & faster hex encoding

fmt.Sprintf("%x", string(s)) creates an intermediate string and performs two redundant conversions ([]bytestring[]byte).
A simpler, allocation-free variant is:

-return json.Marshal(fmt.Sprintf("%x", string(s)))
+return json.Marshal(hex.EncodeToString(s))

It improves clarity and avoids unnecessary copies.


22-33: Gracefully accept 0x-prefixed hex during unmarshal

hex.DecodeString rejects the common 0x… prefix. Adding a quick prefix trim makes the API more robust:

-  str, err := hex.DecodeString(x)
+  x = strings.TrimPrefix(x, "0x")
+  str, err := hex.DecodeString(x)

(Don’t forget the strings import.)
Without this, consumers emitting canonical Ethereum hex strings will fail.

chain/evm/types/chain_config.go (1)

92-106: Name shadowing & unnecessary pointer allocation in getTimeValue

The parameter is called time, shadowing the predeclared time package if imported later.
Also t := time.BigIntMut().Uint64() allocates a new big.Int. You can avoid the extra pointer:

-func getTimeValue(time *sdkmath.Int) *uint64 {
-    if time == nil || time.IsNegative() {
+func getTimeValue(ti *sdkmath.Int) *uint64 {
+    if ti == nil || ti.IsNegative() {
         return nil
     }
-    t := time.BigIntMut().Uint64()
-    return &t
+    v := ti.BigIntMut().Uint64()
+    return &v
 }
chain/evm/types/logs.go (1)

60-63: Genesis / pending logs with block number 0 are rejected

The guard

if log.BlockNumber == 0 {
    return errors.New("block number cannot be zero")
}

makes legitimate logs from the genesis block or pending block impossible.
Recommend allowing 0 and only rejecting negative numbers (which are impossible with uint64).

chain/evm/types/tx_args.go (1)

40-52: String() will panic when hexutil.Big fields are nil

You note the TODO but it is still dangerous: any fmt.Printf("%s", args) on a partially-filled struct causes a runtime panic.
Consider implementing a safer formatter or omit nil-unsafe fields.

-return fmt.Sprintf("TransactionArgs{From:%v, To:%v, Gas:%v,"+
-    " Nonce:%v, Data:%v, Input:%v, AccessList:%v}",
+return fmt.Sprintf("TransactionArgs{From:%v, To:%v, Gas:%v,"+
+    " Nonce:%v, Data:%v, Input:%v, AccessList:%v}",
chain/evm/types/params.go (3)

127-139: Prevent duplicate ExtraEIPs

validateEIPs ensures each ID is activate-able but silently accepts duplicates.
If the same EIP appears twice, subsequent logic that builds maps/sets might waste cycles or mis-count activations. Consider rejecting or deduplicating here.


101-104: Avoid slice aliasing in WithAuthorizedDeployers

The method stores the supplied slice reference directly; callers mutating their slice afterward will unintentionally mutate the Params instance. Copying the slice is safer:

-func (p Params) WithAuthorizedDeployers(authorizedDeployer []string) Params {
-    p.AuthorizedDeployers = authorizedDeployer
+func (p Params) WithAuthorizedDeployers(authorizedDeployer []string) Params {
+    p.AuthorizedDeployers = append([]string(nil), authorizedDeployer...)
     return p
 }

76-80: Clarify Permissioned validation path

Validate() checks ValidateBool(p.Permissioned) even though Permissioned is a strongly-typed bool.
If the intention is to keep parity with the params sub-space (where raw interface{} values flow in), consider adding a comment stating that this call is kept for x/params compatibility; otherwise, the check can be removed.

chain/evm/types/tx_data.go (1)

83-96: Shadowing hides the input pointer – rename for clarity

Inside DeriveChainID, the v := v.Uint64() shadow makes the remaining code block operate on a different type, which is correct but a little opaque. Renaming avoids the shadow entirely:

-if v.BitLen() <= 64 {
-    v := v.Uint64()
-    if v == 27 || v == 28 {
+if v.BitLen() <= 64 {
+    vUint := v.Uint64()
+    if vUint == 27 || vUint == 28 {
         return new(big.Int)
     }
-    if v < 35 {
+    if vUint < 35 {
         return nil
     }
-    return new(big.Int).SetUint64((v - 35) / 2)
+    return new(big.Int).SetUint64((vUint - 35) / 2)
 }
chain/evm/types/dynamic_fee_tx.go (1)

195-199: Minor wording nit – “cannot be nil”

-return errorsmod.Wrap(ErrInvalidGasCap, "gas tip cap cannot nil")
+return errorsmod.Wrap(ErrInvalidGasCap, "gas tip cap cannot be nil")
chain/exchange/types/msgs.go (2)

2149-2153: Function name collision with EVM package

A ValidateAddress helper already exists in chain/evm/types. Re-defining
the same-named function in another types package can confuse IDE symbol
resolution and reviewers.

Consider renaming to something explicit, e.g. ValidateBech32Address, to
avoid ambiguity when both packages are imported.


1180-1206: Code duplication between the two Binary-Options order constructors

NewMsgCreateBinaryOptionsMarketOrderWithScaleFactor is a near copy of
NewMsgCreateBinaryOptionsMarketOrder, differing only by the scale-factor
parameter. Duplicated logic is error-prone.

Extract a private helper:

func newBinaryOptionsMarketOrder(
    sender sdk.AccAddress, marketID string, subaccountID, feeRecipient string,
    price, qty math.LegacyDec, orderType OrderType, margin math.LegacyDec,
) *MsgCreateBinaryOptionsMarketOrder { ... }

Then have each public constructor compute the appropriate margin and call
the helper.

chain/evm/types/access_list_tx.go (2)

26-40: Check SafeNewIntFromBigInt error propagation semantics

Good job using SafeNewIntFromBigInt for bounds checking.
Small nit: the two blocks that cast tx.Value() and tx.GasPrice() are almost identical. Consider extracting a helper to reduce duplication and keep this constructor compact.


82-88: Returning the underlying slice leaks internal state

GetAccessList() returns the slice created by ToEthAccessList().
Although this is already a copy, the method documents “returns the AccessList field”. To avoid accidental mutation by downstream code, return a copy explicitly or document that the caller must not modify the result.

chain/evm/types/msg.go (1)

313-317: UnpackInterfaces uses deprecated msg.Data

msg.Data is guaranteed to be nil by ValidateBasic, so UnpackInterfaces effectively does nothing (and can seg-fault if Any is nil).
Either remove the implementation or unpack msg.Raw instead.

chain/evm/types/utils.go (1)

232-238: HexAddress omits EIP-55 checksum – clarify intent

Lower-case, non-checksummed addresses are fine for internal state-machine paths but unsafe for user-facing APIs.
Make the comment explicit or rename to UnsafeHexAddress to avoid accidental misuse.

chain/exchange/types/exchange.go (2)

32-41: Enum validation helper could be simplified & future-proofed

IsValid() works, but every time you add a new AtomicMarketOrderAccessLevel constant you must remember to extend the switch.
A more defensive implementation is to bound-check against the min / max known values or to auto-generate the string table via go:generate stringer. This completely eliminates the risk of forgetting to update the list and also avoids the subtle “default: false” problem when a new value is appended in the middle.

Example:

func (al AtomicMarketOrderAccessLevel) IsValid() bool {
-   switch al {
-   case AtomicMarketOrderAccessLevel_Nobody,
-       AtomicMarketOrderAccessLevel_SmartContractsOnly,
-       AtomicMarketOrderAccessLevel_BeginBlockerSmartContractsOnly,
-       AtomicMarketOrderAccessLevel_Everyone:
-       return true
-   default:
-       return false
-   }
+   return al >= AtomicMarketOrderAccessLevel_Nobody &&
+          al <= AtomicMarketOrderAccessLevel_Everyone
}

75-81: Duplicate logic between DerivativeOrder and SpotOrder

Both GetFillable() implementations now duplicate the same logic.
Consider declaring an embedded struct (e.g. BaseOrder) that owns OrderInfo and supplies the common getters. That will DRY up the code and make it harder for the two variants to diverge accidentally.

chain/evm/types/errors.go (3)

16-40: Unused error code slot 1 is fine – add a comment explaining the gap

Readers might wonder why the constants start at +2. A clarifying comment such as “// 0 = OK, 1 = internal (reserved by cosmos-sdk)” prevents confusion.


258-273: Potential micro-allocation in NewExecErrorWithReason

common.CopyBytes(revertReason) already allocates; immediately converting to hex again duplicates work. You can avoid the first copy by passing revertReason straight to hexutil.Encode.


275-291: Expose typed accessors on RevertError

Consider adding ReasonHex() (returns string) and ReasonBytes() (returns decoded []byte). This will remove parsing duplication in clients.

🛑 Comments failed to post (29)
chain/exchange/types/positions.go (1)

7-11: 💡 Verification agent

🧩 Analysis chain

Consider adding JSON tags and verify no dangling Position APIs

Only a bare PositionPayout struct remains after the large-scale cleanup.

  1. If the struct is meant to be marshalled (e.g. via JSON‐RPC or REST), lack of tags can break backwards compatibility or produce snake-case field names that differ from existing clients’ expectations.
 type PositionPayout struct {
-	Payout       math.LegacyDec
-	PnlNotional  math.LegacyDec
-	IsProfitable bool
+	Payout       math.LegacyDec `json:"payout"`
+	PnlNotional  math.LegacyDec `json:"pnl_notional"`
+	IsProfitable bool           `json:"is_profitable"`
 }
  1. All former Position methods were removed. Please ensure nothing in v1 code still references them.

🏁 Script executed:

#!/bin/bash
# Quick scan for deleted Position methods that may still be referenced.
rg --line-number -e '\.CopyPosition\(|\.Copy\(.*Position' -e '\.ApplyDelta\(' -e '\.ClosePosition\(' --type go

Length of output: 306


Add JSON tags to PositionPayout and confirm no dangling Position APIs

The PositionPayout struct is exposed over JSON-RPC/REST, so explicit tags will preserve field names and avoid breaking clients:

type PositionPayout struct {
    Payout       math.LegacyDec `json:"payout"`
    PnlNotional  math.LegacyDec `json:"pnl_notional"`
    IsProfitable bool           `json:"is_profitable"`
}

Actions:

  • Update chain/exchange/types/positions.go (lines 7–11) with the above tags.
  • A code scan for CopyPosition, Copy…Position, ApplyDelta and ClosePosition found no usages of the removed Position methods—no dangling APIs remain.
Makefile (1)

7-7: 💡 Verification agent

🧩 Analysis chain

Updated source branch for injective-core.

Switching from a versioned tag to a feature branch (cp-253/reduce-margin-ratio) might introduce instability. Consider pinning to a specific commit hash or moving back to a tagged version once the feature is stable.

Run the following script to check the stability of the branch:


🏁 Script executed:

#!/bin/bash
# Check commit history and activity of the branch
git ls-remote https://github.com/InjectiveLabs/injective-core.git cp-253/reduce-margin-ratio

# Check if this branch has been merged to main/master
git ls-remote https://github.com/InjectiveLabs/injective-core.git main
git ls-remote https://github.com/InjectiveLabs/injective-core.git master

echo "Check if the branch has significant recent activity that might indicate it's still changing"

Length of output: 777


Fix repository URL and pin feature branch commit

It looks like the Makefile is trying to clone from https://github.com/InjectiveLabs/injective-core.git, which returns “repository not found.” Please:

  • Verify and update the GitHub repo path (e.g. injective-labs/injective-core.git if that’s the correct owner).
  • Ensure the branch cp-253/reduce-margin-ratio actually exists on that repo.
  • To avoid instability, pin the clone to a specific commit hash (or switch back to a tagged release) once the feature is stable.
chain/erc20/types/params.go (1)

12-14: ⚠️ Potential issue

Inconsistent parameter handling in constructor.

The NewParams function accepts a wasmHookQueryMaxGas parameter but doesn't use it when constructing the Params struct, which appears to be empty. This suggests either:

  1. The parameter is intended for future use but not yet implemented
  2. This is a copy-paste error from another module

This creates confusion about the intended parameter structure.

If the parameter is not needed, remove it:

-func NewParams(wasmHookQueryMaxGas uint64) Params {
+func NewParams() Params {
   return Params{}
}

If it should be used, update the Params struct and constructor accordingly.

chain/erc20/types/msgs.go (1)

75-81: ⚠️ Potential issue

MsgDeleteTokenPair should validate BankDenom field.

The ValidateBasic method only validates the sender address but doesn't check the BankDenom field, which could lead to deleting a token pair with an invalid or empty bank denomination.

func (m MsgDeleteTokenPair) ValidateBasic() error {
    if _, err := sdk.AccAddressFromBech32(m.Sender); err != nil {
        return err
    }

+   // Validate bank denom is not empty
+   if strings.TrimSpace(m.BankDenom) == "" {
+       return errorsmod.Wrap(ErrInvalidDenom, "bank denom cannot be empty")
+   }
+
    return nil
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

func (m MsgDeleteTokenPair) ValidateBasic() error {
	if _, err := sdk.AccAddressFromBech32(m.Sender); err != nil {
		return err
	}

	// Validate bank denom is not empty
	if strings.TrimSpace(m.BankDenom) == "" {
		return errorsmod.Wrap(ErrInvalidDenom, "bank denom cannot be empty")
	}

	return nil
}
chain/erc20/types/types.go (1)

24-52: ⚠️ Potential issue

Missing validation case for DenomTypeEVM

While the GetDenomType function handles the "evm/" prefix, there's no corresponding validation case for DenomTypeEVM in the Validate method's switch statement. This could lead to EVM denominations falling through to the default error case.

Add a specific case for EVM denomination validation:

func (pair *TokenPair) Validate() error {
	switch GetDenomType(pair.BankDenom) {
+	case DenomTypeEVM:
+		// Add appropriate validation for EVM denominations
+		if !strings.HasPrefix(pair.BankDenom, "evm/") {
+			return errors.Wrapf(ErrInvalidTokenPair, "token pair has invalid evm denom")
+		}
	case DenomTypeTokenFactory:
		if _, _, err := tftypes.DeconstructDenom(pair.BankDenom); err != nil {
			return errors.Wrapf(ErrInvalidTokenPair, "token pair has invalid tokenfactory denom: %s", err.Error())
		}
chain/evm/types/compiled_contract.go (1)

109-116: ⚠️ Potential issue

Incorrect variable checked – possible silent contract-load failure

After unmarshalling simpleStorageJSON the code validates TestMessageCall.Bin again instead of SimpleStorageContract.Bin:

-  if len(TestMessageCall.Bin) == 0 {
+  if len(SimpleStorageContract.Bin) == 0 {
       panic("load contract failed")
   }

As written, a broken SimpleStorageContract.json would go unnoticed, leading to panics later at runtime.

📝 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.

	err = json.Unmarshal(simpleStorageJSON, &SimpleStorageContract)
	if err != nil {
		panic(err)
	}

	if len(SimpleStorageContract.Bin) == 0 {
		panic("load contract failed")
	}
chain/exchange/types/spot_orders.go (1)

64-66: ⚠️ Potential issue

Convert 32-byte arrays to slices before writing to Keccak hash

sha3.Writer.Write expects []byte; passing an array causes a type mismatch.

- w.Write(domainSeparator)
- w.Write(typedDataHash)
+ w.Write(domainSeparator[:])
+ w.Write(typedDataHash[:])

Without the slice conversions the code will not compile.

📝 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.

    w.Write(domainSeparator[:])
    w.Write(typedDataHash[:])
chain/evm/types/int.go (2)

20-23: 🛠️ Refactor suggestion

MaxInt256 escapes via a mutable pointer – risk of unintended global mutation

MaxInt256 holds a pointer to a big.Int that was defined as a local variable (tmp) inside init.
Down-stream code can mutate that big.Int, accidentally changing the “constant” for the whole process (and causing data races if mutated concurrently).

Consider publishing an immutable copy or returning fresh copies on demand.

-var MaxInt256 *big.Int
+var maxInt256Val big.Int   // unexported – real storage
+var MaxInt256 = new(big.Int) // exported, safe copy helper
func init() {
    maxInt256Val.SetBit(&maxInt256Val, sdkmath.MaxBitLen, 1).Sub(&maxInt256Val, big.NewInt(1))
    MaxInt256 = new(big.Int).Set(&maxInt256Val) // copy, stays read-only
}

50-56: ⚠️ Potential issue

SaturatedNewInt aliases the global MaxInt256 – causes shared mutation

When the input exceeds 256 bits, you assign i = MaxInt256 and pass that pointer to sdkmath.NewIntFromBigInt.
math.Int stores the same pointer, so subsequent arithmetic on the returned Int will mutate MaxInt256.

-    i = MaxInt256
+    i = new(big.Int).Set(MaxInt256) // create an independent copy
chain/evm/types/chain_config.go (2)

20-24: ⚠️ Potential issue

Nil-pointer panic if EIP155ChainID is unset

cc.EIP155ChainID.BigIntMut() will panic when EIP155ChainID == nil.
Either guarantee it is always initialised (validate) or guard here.

-        ChainID:                 cc.EIP155ChainID.BigIntMut(),
+        ChainID: func() *big.Int {
+            if cc.EIP155ChainID == nil {
+                return nil // or big.NewInt(0)
+            }
+            return cc.EIP155ChainID.BigIntMut()
+        }(),
📝 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 (cc ChainConfig) EthereumConfig() *params.ChainConfig {
    cfg := &params.ChainConfig{
        ChainID: func() *big.Int {
            if cc.EIP155ChainID == nil {
                return nil // or big.NewInt(0)
            }
            return cc.EIP155ChainID.BigIntMut()
        }(),
        HomesteadBlock:          getBlockValue(cc.HomesteadBlock),
        DAOForkBlock:            getBlockValue(cc.DAOForkBlock),
        // …
    }
    // …
}

108-172: 🛠️ Refactor suggestion

Validate() omits check for mandatory EIP155ChainID & nil-safety

Validation never verifies that EIP155ChainID is present and positive.
Add a first–class check to fail early, and re-use it later when calling EthereumConfig().

 func (cc ChainConfig) Validate() error {
+    if cc.EIP155ChainID == nil || !cc.EIP155ChainID.IsPositive() {
+        return errorsmod.Wrap(ErrInvalidChainConfig, "EIP155ChainID must be set and positive")
+    }

Committable suggestion skipped: line range outside the PR's diff.

chain/evm/types/tx_args.go (1)

116-147: ⚠️ Potential issue

EIP-1559 fee fields are never propagated to the message

ToMessage initialises gasFeeCap and gasTipCap from gasPrice, not from MaxFeePerGas / MaxPriorityFeePerGas.
For 1559 transactions these caps will incorrectly be zero, causing under-priced txs.

-    gasPrice = new(big.Int)
-    if args.GasPrice != nil {
-        gasPrice = args.GasPrice.ToInt()
-    }
-    gasFeeCap, gasTipCap = gasPrice, gasPrice
+    if args.MaxFeePerGas != nil {           // EIP-1559 style
+        gasFeeCap = args.MaxFeePerGas.ToInt()
+        if args.MaxPriorityFeePerGas != nil {
+            gasTipCap = args.MaxPriorityFeePerGas.ToInt()
+        } else {
+            gasTipCap = new(big.Int)
+        }
+    } else {
+        gasPrice = new(big.Int)
+        if args.GasPrice != nil {
+            gasPrice = args.GasPrice.ToInt()
+        }
+        gasFeeCap, gasTipCap = gasPrice, gasPrice
+    }
chain/evm/types/params.go (1)

154-172: 🛠️ Refactor suggestion

Normalize & de-duplicate authorised deployer addresses

validateAuthorizedDeployers currently regards 0xdead… and 0xDEAD… as different strings, so the duplicate check can be bypassed with mixed-case inputs.
Converting every entry to its canonical EIP-55 checksum form before validation removes this loophole and simplifies later address comparisons.

-for _, addrStr := range v {
-    if !common.IsHexAddress(addrStr) {
-        return fmt.Errorf("invalid address: %s", addrStr)
-    }
-
-    if _, found := authorisedAddresses[addrStr]; found {
-        return fmt.Errorf("duplicate authorised address: %s", addrStr)
-    }
-
-    authorisedAddresses[addrStr] = struct{}{}
+for _, addrStr := range v {
+    // Convert to checksum to make comparisons case-insensitive
+    normalised := common.HexToAddress(addrStr).Hex()
+
+    if !common.IsHexAddress(normalised) {
+        return fmt.Errorf("invalid address: %s", addrStr)
+    }
+    if _, found := authorisedAddresses[normalised]; found {
+        return fmt.Errorf("duplicate authorised address: %s", addrStr)
+    }
+    authorisedAddresses[normalised] = struct{}{}
 }

Committable suggestion skipped: line range outside the PR's diff.

chain/evm/types/dynamic_fee_tx.go (3)

178-192: 🛠️ Refactor suggestion

SetSignatureValues should bounds-check chainID

chainID is converted straight into sdkmath.Int without the
IsValidInt256 guard used elsewhere (SafeNewIntFromBigInt). An oversized
value will silently overflow and later arithmetic may panic.

-    if chainID != nil {
-        chainIDInt := sdkmath.NewIntFromBigInt(chainID)
-        tx.ChainID = &chainIDInt
-    }
+    if chainID != nil {
+        if !IsValidInt256(chainID) {
+            // keep behaviour consistent with other validators
+            panic(errorsmod.Wrap(ErrInvalidAmount, "chainID overflows 256-bit"))
+        }
+        chainIDInt := sdkmath.NewIntFromBigInt(chainID)
+        tx.ChainID = &chainIDInt
+    }

Committable suggestion skipped: line range outside the PR's diff.


65-77: 🛠️ Refactor suggestion

Deep-copy Accesses to avoid accidental shared mutation

tx.Accesses is a pointer.
Returning the same pointer in the copy means any in-place modification of the
copied value will leak into the original and vice-versa, which breaks the
expectation of Copy() producing an independent object.

-        Accesses:  tx.Accesses,
+        Accesses:  func() *AccessList {
+            if tx.Accesses == nil {
+                return nil
+            }
+            alCopy := *tx.Accesses
+            return &alCopy
+        }(),
📝 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 (tx *DynamicFeeTx) Copy() TxData {
	return &DynamicFeeTx{
		ChainID:   tx.ChainID,
		Nonce:     tx.Nonce,
		GasTipCap: tx.GasTipCap,
		GasFeeCap: tx.GasFeeCap,
		GasLimit:  tx.GasLimit,
		To:        tx.To,
		Amount:    tx.Amount,
		Data:      common.CopyBytes(tx.Data),
		Accesses:  func() *AccessList {
			if tx.Accesses == nil {
				return nil
			}
			alCopy := *tx.Accesses
			return &alCopy
		}(),
		V:         common.CopyBytes(tx.V),
		R:         common.CopyBytes(tx.R),
	}
}

267-269: ⚠️ Potential issue

Potential nil-pointer dereference in EffectiveGasPrice

GasFeeCap and GasTipCap may be nil (e.g. when called on an
un-validated instance). Calling .BigInt() will panic.

-func (tx *DynamicFeeTx) EffectiveGasPrice(baseFee *big.Int) *big.Int {
-    return EffectiveGasPrice(baseFee, tx.GasFeeCap.BigInt(), tx.GasTipCap.BigInt())
+func (tx *DynamicFeeTx) EffectiveGasPrice(baseFee *big.Int) *big.Int {
+    var feeCap, tipCap *big.Int
+    if tx.GasFeeCap != nil {
+        feeCap = tx.GasFeeCap.BigInt()
+    }
+    if tx.GasTipCap != nil {
+        tipCap = tx.GasTipCap.BigInt()
+    }
+    return EffectiveGasPrice(baseFee, feeCap, tipCap)
 }
chain/evm/types/legacy_tx.go (1)

50-60: 🛠️ Refactor suggestion

Copy performs shallow cloning of pointer fields

GasPrice, Amount, and the signature slices still point to the original
memory. Mutating the copy will mutate the original.

Same pattern as in DynamicFeeTx.Copy() – consider deep copying or
documenting that the method returns a shallow copy only.

chain/exchange/types/msgs.go (1)

2155-2171: ⚠️ Potential issue

hasDuplicatesOrder treats empty strings as legitimate keys

The maps always store entries even when hash or cid are empty; this means
only two empty hashes/cids are required to be considered unique, which
defeats the purpose of the duplicate check.

-    if (hash != "" && hashExists) || (cid != "" && cidExists) {
+    if (hash != "" && hashExists) || (cid != "" && cidExists) {
         return true
     }
-    seenHashes[hash] = struct{}{}
-    seenCids[cid] = struct{}{}
+    if hash != "" {
+        seenHashes[hash] = struct{}{}
+    }
+    if cid != "" {
+        seenCids[cid] = struct{}{}
+    }
chain/evm/types/access_list_tx.go (2)

58-70: 🛠️ Refactor suggestion

⚠️ Potential issue

Copy() shares the same AccessList slice → potential aliasing

tx.Accesses is copied by reference, therefore the returned transaction and the original still point to the same underlying slice.
A caller mutating the copy (e.g. copyTx.Accesses[0].StorageKeys = …) would silently mutate the source, which is unexpected for a copy operation.

-       Accesses: tx.Accesses,
+       Accesses: append(AccessList(nil), tx.Accesses...), // shallow-copy slice header & backing array

If AccessList itself contains pointers you may need a deeper copy.

📝 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.

	return &AccessListTx{
		ChainID:  tx.ChainID,
		Nonce:    tx.Nonce,
		GasPrice: tx.GasPrice,
		GasLimit: tx.GasLimit,
		To:       tx.To,
		Amount:   tx.Amount,
		Data:     common.CopyBytes(tx.Data),
	-	Accesses: tx.Accesses,
+		Accesses: append(AccessList(nil), tx.Accesses...), // shallow-copy slice header & backing array
		V:        common.CopyBytes(tx.V),
		R:        common.CopyBytes(tx.R),
		S:        common.CopyBytes(tx.S),
	}

182-207: 🛠️ Refactor suggestion

Gas-limit is not validated

Validate() ensures gasPrice, amount, and the fee’s 256-bit bounds, but never checks that GasLimit is > 0.
A zero gas-limit transaction will fail in the EVM anyway; surfacing this earlier gives friendlier error messages.

+   if tx.GasLimit == 0 {
+       return errorsmod.Wrap(errortypes.ErrInvalidGas, "gas limit cannot be 0")
+   }
📝 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 (tx AccessListTx) Validate() error {
	if tx.GasLimit == 0 {
		return errorsmod.Wrap(errortypes.ErrInvalidGas, "gas limit cannot be 0")
	}

	gasPrice := tx.GetGasPrice()
	if gasPrice == nil {
		return errorsmod.Wrap(ErrInvalidGasPrice, "cannot be nil")
	}
	if !IsValidInt256(gasPrice) {
		return errorsmod.Wrap(ErrInvalidGasPrice, "out of bound")
	}

	if gasPrice.Sign() == -1 {
		return errorsmod.Wrapf(ErrInvalidGasPrice, "gas price cannot be negative %s", gasPrice)
	}

	amount := tx.GetValue()
	// Amount can be 0
	if amount != nil && amount.Sign() == -1 {
		return errorsmod.Wrapf(ErrInvalidAmount, "amount cannot be negative %s", amount)
	}
	if !IsValidInt256(amount) {
		return errorsmod.Wrap(ErrInvalidAmount, "out of bound")
	}

	if !IsValidInt256(tx.Fee()) {
		return errorsmod.Wrap(ErrInvalidGasFee, "out of bound")
	}

	// ... rest of Validate()
	return nil
}
chain/evm/types/msg.go (3)

223-243: 🛠️ Refactor suggestion

Hard-coded SIGN_MODE_TEXTUAL may break hardware-wallet flow

keyring.Signer.SignByAddress with SIGN_MODE_TEXTUAL assumes every signer supports that mode.
Ledger & Trezor apps usually only implement SIGN_MODE_LEGACY_AMINO_JSON or SIGN_MODE_ETHEREUM. Falling back when SIGN_MODE_TEXTUAL is not supported avoids user-facing failures.


331-361: ⚠️ Potential issue

Zero-fee tx results in empty slice → builder error

builder.SetFeeAmount(fees) is always executed, but fees may be an empty slice when GetFee()==0 (e.g. dynamic-fee tx with gasPrice==0).
auth/tx rejects an empty fee set. Guard the call:

-if err := builder.SetMsgs(&MsgEthereumTx{…}); err != nil { … }
-builder.SetFeeAmount(fees)
+if err := builder.SetMsgs(&MsgEthereumTx{…}); err != nil { … }
+if len(fees) > 0 {
+    builder.SetFeeAmount(fees)
+}
📝 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.

// BuildTx builds the canonical cosmos tx from ethereum msg
func (msg *MsgEthereumTx) BuildTx(b client.TxBuilder, evmDenom string) (authsigning.Tx, error) {
    builder, ok := b.(authtx.ExtensionOptionsTxBuilder)
    if !ok {
        return nil, errors.New("unsupported builder")
    }

    option, err := codectypes.NewAnyWithValue(&ExtensionOptionsEthereumTx{})
    if err != nil {
        return nil, err
    }

    fees := make(sdk.Coins, 0)
    fee := msg.GetFee()
    feeAmt := sdkmath.NewIntFromBigInt(fee)
    if feeAmt.Sign() > 0 {
        fees = append(fees, sdk.NewCoin(evmDenom, feeAmt))
    }

    builder.SetExtensionOptions(option)

    if err := builder.SetMsgs(&MsgEthereumTx{
        From: msg.From,
        Raw:  msg.Raw,
    }); err != nil {
        return nil, err
    }
    if len(fees) > 0 {
        builder.SetFeeAmount(fees)
    }
    builder.SetGasLimit(msg.GetGas())
    return builder.GetTx(), nil
}

70-113: ⚠️ Potential issue

Branch selection ignores tipCap without feeCap

gasTipCap is irrelevant unless gasFeeCap is supplied, but callers that only fill tipCap fall through to the AccessList / Legacy branches, producing an invalid 1559 transaction (tip ignored). Consider:

-case gasFeeCap != nil:
+case gasFeeCap != nil || gasTipCap != nil:

and error if only one of the pair is set.

chain/evm/types/errors.go (3)

114-124: ⚠️ Potential issue

Missing Unwrap() method prevents errors.Is/As inspection

Since Go 1.20 the canonical way to expose an inner error is:

func (e *vmErrorWithRet) Unwrap() error { return e.cause }

Without it, callers must rely on the custom Cause() helper, which breaks errors.Is/As interoperability and log filtering.

Please add Unwrap() so that sdkerrors.IsOf() et al. keep working.


222-256: ⚠️ Potential issue

NewVMErrorWithRet does not set cause when UnpackRevert fails

If ABI-unpacking fails, e.cause stays nil, so ABCI Info() will emit an empty error code even though execution reverted.

Move e.cause assignment outside of the errUnpack == nil guard:

if e.vmErr == vm.ErrExecutionReverted.Error() {
    e.err = vm.ErrExecutionReverted
    e.ret = common.CopyBytes(ret)
    reason, errUnpack := abi.UnpackRevert(e.ret)
-   if errUnpack == nil {
-       e.err   = fmt.Errorf("%s: %s", e.err.Error(), reason)
-       e.reason = reason
-       e.cause  = errorsmod.Wrap(ErrExecutionReverted, e.toJSON())
-   }
+   if errUnpack == nil {
+       e.err   = fmt.Errorf("%s: %s", e.err.Error(), reason)
+       e.reason = reason
+   }
+   e.cause = errorsmod.Wrap(ErrExecutionReverted, e.toJSON())
} else {

This guarantees modules can always errors.Is(err, types.ErrExecutionReverted).


203-212: 🛠️ Refactor suggestion

Binary ret field is marshalled as base64, not hex

json.Marshal on []byte encodes to base64. Down-stream consumers (CLI, indexers) typically expect hex-encoded revert data to match JSON-RPC eth_call semantics.

Two options:

  1. Change the field to string and pre-encode via hexutil.Encode(e.ret).
  2. Keep []byte but document the base64 format and update all consumers.
-type abciLogVMError struct {
-    ...
-    Ret     []byte `json:"ret,omitempty"`
+type abciLogVMError struct {
+    ...
+    Ret     string `json:"ret,omitempty"` // hex-encoded
 }
 ...
 logData := &abciLogVMError{
     ...
-    Ret:     e.ret,
+    Ret:     hexutil.Encode(e.ret),
 }
chain/exchange/types/market.go (2)

76-84: ⚠️ Potential issue

Division loses precision for large decimal offsets

math.LegacyDec.Quo() performs integer division with the library’s precision (18 decimals by default). For > 18-digit multipliers the result is silently truncated.

Either:

  1. Switch to sdk.Dec with dynamic precision, or
  2. Perform the division on math.Int then convert to LegacyDec.

Failing to do so will mis-price pairs with tokens like USDT (6) vs BTC (8).


86-100: 🛠️ Refactor suggestion

**Symmetry between _FromChainFormat and _ToChainFormat is not guaranteed

Because of the truncation mentioned above, ToChainFormat(FromChainFormat(x)) != x for some values.

Add unit tests asserting round-trip equality for a range of values and decimal pairs. This will surface precision issues early.

chain/exchange/types/derivative_orders.go (1)

77-97: 🛠️ Refactor suggestion

Order hash computation with price scaling

The test-only method properly handles price format conversion before hash computation. However, there's a side effect where the method modifies the order's price and margin values directly.

Consider making a copy of the values or using local variables to avoid modifying the original order:

- m.OrderInfo.Price = PriceFromChainFormat(m.OrderInfo.Price, 0, scaleFactor)
- m.Margin = NotionalFromChainFormat(m.Margin, scaleFactor)
+ scaledPrice := PriceFromChainFormat(m.OrderInfo.Price, 0, scaleFactor)
+ scaledMargin := NotionalFromChainFormat(m.Margin, scaleFactor)

  return ComputeOrderHash(
    m.MarketId,
    m.OrderInfo.SubaccountId,
    m.OrderInfo.FeeRecipient,
-   m.OrderInfo.Price.String(),
+   scaledPrice.String(),
    m.OrderInfo.Quantity.String(),
-   m.Margin.String(),
+   scaledMargin.String(),
    triggerPrice,
    string(m.OrderType),
    nonce)
📝 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 *DerivativeOrder) ComputeOrderHash(nonce, scaleFactor uint32) (common.Hash, error) {
	triggerPrice := ""
	if m.TriggerPrice != nil {
		triggerPrice = m.TriggerPrice.String()
	}

	scaledPrice := PriceFromChainFormat(m.OrderInfo.Price, 0, scaleFactor)
	scaledMargin := NotionalFromChainFormat(m.Margin, scaleFactor)

	return ComputeOrderHash(
		m.MarketId,
		m.OrderInfo.SubaccountId,
		m.OrderInfo.FeeRecipient,
		scaledPrice.String(),
		m.OrderInfo.Quantity.String(),
		scaledMargin.String(),
		triggerPrice,
		string(m.OrderType),
		nonce)
}

@aarmoa
Copy link
Collaborator

aarmoa commented May 13, 2025

@maxim-inj I think I have added all these changes already in #263
Could you check if everything is covered? If nothing is missing we can close this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants