Chore/sync dev with master v1.57 #307
Conversation
* feat: removed the deprecated pre-commit repo pre-commit-golang. Added new CI step to run golangci-lint using the github action * fix: added skip directories into the golanci lint config * fix: fixed lint errors * fix: fixed lint errors * feat: updated proto definitions for the chain upgrade v1.15 * fix: synced go.mod dependecnies with Injective core * fix: fixed lint errors * fix: fixed lint errors * feat: updated all proto definitions for v1.15 chain upgrade. Added an example for the new GetEipBaseFee request in txfees module * feat: updated all examples to query gas price from the chain instead of using the default value * Feat/sync with master v1.56.2 (#298) * (fix) Applied the fixes for EIP712 JSON unmarshalling to the master branch * fix: update pre-commit/action from v2.0.2 to v3.0.1 to ry to solve the pre-commit validation workflow error * fix: Added configuration in .golangci.yml file to exclude some dirs * fix: updated the oracle stream prices script to send the oracle type always in lowercase * fix: updated the oracle stream prices script to send the oracle type always in lowercase * fix: Added quantization in the functions that convert notional values to chain format * fix/fix_manual_tx_generation (#296) * fix: fixed the manual TX generation and signing to receive the gas price * fix: solve issues in pre-commit workflow caused by using deprecated components * fix: solve issues in pre-commit workflow caused by using deprecated components * feat: updated proto definitions for the chain upgrade v1.15 * (fix) Applied the fixes for EIP712 JSON unmarshalling to the master branch * fix: updated the oracle stream prices script to send the oracle type always in lowercase * fix: updated the oracle stream prices script to send the oracle type always in lowercase * fix: synced go.mod dependecnies with Injective core * fix: fixed lint errors * fix: fixed lint errors * feat: updated all proto definitions for v1.15 chain upgrade. Added an example for the new GetEipBaseFee request in txfees module * feat: updated all examples to query gas price from the chain instead of using the default value * fix: fix function parameter type * feat: updated proto definition to v1.15.0-beta.2 and v1.14.48 for Indexer * fix: update ofac.json file * Improve error messaging and enhance documentation in eip712_cosmos.go (#240) This PR improves error handling and enhances documentation in the eip712_cosmos.go file to provide clearer messages and more detailed explanations. Signed-off-by: Hwangjae Lee <meetrick@gmail.com> Co-authored-by: Abel Armoa <30988000+aarmoa@users.noreply.github.com> * fix: fixed array creatin in a unit test (#299) * fix: updated chain version to v1.15 and indexer version to v1.15.6 * fix: removed explicit inclusion of txfees response in the context initialization (not required) --------- Signed-off-by: Hwangjae Lee <meetrick@gmail.com> Co-authored-by: Hwangjae Lee <meetrick@gmail.com>
WalkthroughTwo new struct types have been introduced: Changes
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 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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: 17
🔭 Outside diff range comments (1)
Makefile (1)
186-186: 💡 Verification agent🧩 Analysis chain
Fix the failing coverage target.
The pipeline failure indicates the coverage target is failing with a panic and exit code 2. This needs to be addressed to ensure proper test coverage reporting.
Investigate and fix the test command that's causing the coverage target to fail. The error suggests there may be:
- Test panics in the codebase
- Import path issues after the sync
- Missing dependencies or initialization problems
Run this script to identify the specific test failures:
🏁 Script executed:
#!/bin/bash # Description: Identify failing tests in coverage target # Run tests with verbose output to see specific failures go test -race -v ./client/... ./ethereum/... 2>&1 | head -50 # Check for common issues echo "=== Checking for import issues ===" fd -e go -x grep -l "github.com/InjectiveLabs/injective-core" {} \; | head -10 echo "=== Checking for missing dependencies ===" go mod tidy go mod verifyLength of output: 18902
Enable CGO or remove the race flag in the coverage target
The failure isn’t caused by panics or missing imports, but by using
-racewithout CGO enabled. The race detector requiresCGO_ENABLED=1(and a C compiler) in CI. To fix, update the Makefile at line 186:• File: Makefile (line 186)
- Either enable CGO for the race detector:
- go test -race -coverprofile=coverage.out -covermode=atomic ./client/... ./ethereum/... + CGO_ENABLED=1 go test -race -coverprofile=coverage.out -covermode=atomic ./client/... ./ethereum/...- Or drop the
-raceflag if you only need coverage reporting:- go test -race -coverprofile=coverage.out -covermode=atomic ./client/... ./ethereum/... + go test -coverprofile=coverage.out -covermode=atomic ./client/... ./ethereum/...🧰 Tools
🪛 GitHub Actions: run-tests
[error] 186-186: Make coverage target failed due to test command panic and exit code 2.
🧹 Nitpick comments (22)
chain/exchange/types/common_order.go (1)
83-89: Consider the necessity of these simple getter methods.The new
GetPrice()andGetQuantity()methods are simple accessors that directly return the struct fields. In Go, direct field access is often preferred over getters unless there's specific logic or future extensibility requirements.If these getters don't provide additional logic or interface compliance, consider using direct field access:
-func (m *OrderInfo) GetPrice() math.LegacyDec { - return m.Price -} - -func (m *OrderInfo) GetQuantity() math.LegacyDec { - return m.Quantity -}chain/evm/types/storage.go (1)
32-40: Consider using strings.Builder for better performance.For large storage with many states, string concatenation using
+=can be inefficient due to repeated allocations.func (s Storage) String() string { - var str string + var builder strings.Builder for _, state := range s { - str += fmt.Sprintf("%s\n", state.String()) + builder.WriteString(fmt.Sprintf("%s\n", state.String())) } - return str + return builder.String() }.golangci.yml (1)
4-18: Consider re-enablingstaticcheckorgoimportsDisabling those linters removes detection of unused code, nil slice/map checks, and import-ordering fixes that frequently catch subtle bugs during refactors of this size.
If build time is a concern, you can keep them disabled inrun.fast: truemode or limit the affected directories instead of a blanket disable.chain/exchange/types/common_utils.go (1)
311-320:HasDuplicatesis case-sensitive – confirm this is intendedTwo denoms differing only by case (
usdtvsUSDT) are treated as distinct.
If denoms are expected to be case-insensitive (as with most chain denoms), normalise withstrings.ToLower.chain/evm/types/tx_data.go (1)
19-19: TODO comment suggests future improvement.The TODO comment indicates a plan to embed
ethtypes.TxDatawhich would align better with go-ethereum's interface. Consider tracking this as a separate issue for future enhancement.Would you like me to create a GitHub issue to track this TODO item?
chain/exchange/types/authz_exchange_generic.go (1)
140-147: Consider type-safe alternative to context.Value.Using
context.Valuefor passing hold amounts is not type-safe and can lead to runtime panics if the wrong type is stored.Consider defining a custom context type or using a more type-safe approach:
type HoldContext struct { context.Context hold sdk.Coins } func WithHold(ctx context.Context, hold sdk.Coins) *HoldContext { return &HoldContext{Context: ctx, hold: hold} } func (hc *HoldContext) GetHold() (sdk.Coins, bool) { return hc.hold, true }chain/evm/types/legacy_tx.go (2)
44-47: Return type inconsistency in TxType method.The
TxType()method returnsuint8but theTxDatainterface defines it as returningbyte. While these are equivalent types, it's better to be consistent with the interface.Apply this fix for consistency:
-func (tx *LegacyTx) TxType() uint8 { +func (tx *LegacyTx) TxType() byte { return ethtypes.LegacyTxType }
123-125: Misleading comment in AsEthereumData method.The comment mentions "AccessListTx transaction" but this method is for legacy transactions.
Fix the comment:
-// AsEthereumData returns an AccessListTx transaction tx from the proto-formatted +// AsEthereumData returns a LegacyTx transaction tx from the proto-formatted // TxData defined on the Cosmos EVM.chain/evm/types/params.go (2)
17-22: Clarify documentation ofDefaultAllowUnprotectedTxs.
The sentence “rejects all unprotected txs (true = allow, false = reject)” is contradictory – “rejects” while the default value istrue, meaning allow. A clearer phrasing will avoid confusion for downstream integrators and tooling.- // DefaultAllowUnprotectedTxs rejects all unprotected txs (true = allow, false = reject) + // DefaultAllowUnprotectedTxs defines whether unprotected (non-EIP-155) transactions are + // accepted (`true`) or rejected (`false`). The default is `true` (accepted).
127-139:validateEIPsdoes not guard against duplicate EIPs.
Supplying the same EIP twice presently passes validation but will later be turned into duplicate entries inExtraEIPs, wasting gas and increasing cognitive load. A quick set check will eliminate accidental duplication.chain/evm/types/access_list_tx.go (2)
28-43: Avoid unnecessary heap allocations when taking the address of local variables.
amountIntandgasPriceIntare stack variables whose addresses escape. The compiler will move them to the heap, but you can avoid the allocation by constructing the value directly:-amountInt, err := types.SafeNewIntFromBigInt(tx.Value()) +amount, err := types.SafeNewIntFromBigInt(tx.Value()) ... -txData.Amount = &amountInt +txData.Amount = &amountSame for
gasPriceInt. The change is minor but helps keep allocations predictable in tight loops (e.g. relayer ingress).
185-188:GasPricemust never benil– consider failing fast in the constructor.
Validaterightfully rejects anilgas price, yetNewAccessListTxaccepts such a transaction and defers the check. Detecting the error earlier surfaces feedback to callers sooner and prevents them from manipulating a partially-formed object.if tx.GasPrice() == nil { return nil, errorsmod.Wrap(ErrInvalidGasPrice, "gas price cannot be nil for AccessList tx") }chain/evm/types/dynamic_fee_tx.go (1)
196-206: Minor wording & nil-safety nitpicks in validation errors
- Error strings omit “be” – should read “cannot be nil” for clarity.
types.IsValidInt256(tx.Fee())is called even whentx.Fee()may returnnil(e.g. malformed object built outsideNewDynamicFeeTx).
Guard with a nil check to avoid a panic if the helper doesn’t toleratenil.- return errorsmod.Wrap(ErrInvalidGasCap, "gas tip cap cannot nil") + return errorsmod.Wrap(ErrInvalidGasCap, "gas tip cap cannot be nil") - if !types.IsValidInt256(tx.Fee()) { + if fee := tx.Fee(); fee == nil || !types.IsValidInt256(fee) { return errorsmod.Wrap(ErrInvalidGasFee, "out of bound") }chain/evm/types/utils.go (1)
21-24: Typo in comment
pratical➜practicalchain/evm/types/errors.go (1)
238-254: Minor optimisation – avoid double JSON marshaling on every VM error
NewVMErrorWithRetcallse.toJSON()twice (once forerrcomposition and once forcausewrapping).
Serialising large revert data twice could be expensive. Cache once:- e.err = fmt.Errorf("%s: %s", e.err.Error(), reason) - e.reason = reason - e.cause = errorsmod.Wrap(ErrExecutionReverted, e.toJSON()) + e.err = fmt.Errorf("%s: %s", e.err.Error(), reason) + e.reason = reason + jsonStr := e.toJSON() + e.cause = errorsmod.Wrap(ErrExecutionReverted, jsonStr)chain/evm/types/msg.go (1)
281-292: Avoid unnecessary allocations & nil-copy inAsMessage
new(big.Int).Set(tx.GasPrice())creates an extra allocation and triggers the internal nil-check for every field. A small optimisation & safety tweak:- GasPrice: new(big.Int).Set(tx.GasPrice()), - GasFeeCap: new(big.Int).Set(tx.GasFeeCap()), - GasTipCap: new(big.Int).Set(tx.GasTipCap()), + GasPrice: big.NewInt(0).Set(tx.GasPrice()), // Set handles nil => 0 + GasFeeCap: big.NewInt(0).Set(tx.GasFeeCap()), + GasTipCap: big.NewInt(0).Set(tx.GasTipCap()),chain/exchange/types/positions.go (1)
7-11: Exported struct lacks JSON / Proto tags
PositionPayoutis now exported but its fields have no struct tags, making JSON / protobuf (un-)marshalling default to the ugly field names (Payout,PnlNotional, …). If this type leaves the module boundary (e.g. in queries), consider:type PositionPayout struct { - Payout math.LegacyDec - PnlNotional math.LegacyDec - IsProfitable bool + Payout math.LegacyDec `json:"payout" protobuf:"bytes,1,opt,name=payout"` + PnlNotional math.LegacyDec `json:"pnl_notional" protobuf:"bytes,2,opt,name=pnl_notional"` + IsProfitable bool `json:"is_profitable" protobuf:"varint,3,opt,name=is_profitable"` }chain/exchange/types/proposal.go (1)
13-15: Duplicate GOV import – unify to a single aliasBoth
govtypes(v1beta1) andgov(legacy) packages are imported. This is error-prone and forces devs to remember which alias exposes which constant.If you only need
gov.ErrInvalidProposalContent, import it explicitly with the existinggovtypesalias to keep the import section lean:- sdk "github.com/cosmos/cosmos-sdk/types" - gov "github.com/cosmos/cosmos-sdk/x/gov/types" + sdk "github.com/cosmos/cosmos-sdk/types" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"(Adjust references from
gov.togovtypes.)chain/exchange/types/market.go (1)
64-112: Add bound-checking fordecimalsargumentsAll helpers trust the caller to pass sane
decimals(≤MaxDecimals). A wrong value can allocate abig.Intwith millions of digits and DOS the node.Consider clamping or validating the argument once:
if decimals > MaxDecimals { panic(fmt.Errorf("decimals %d exceeds MaxDecimals(%d)", decimals, MaxDecimals)) }chain/exchange/types/msgs.go (2)
287-302: Trigger-price validation logic inverted?The intention seems to be “allow nil or zero; reject any other
value”. The conditionif o.TriggerPrice != nil && (o.TriggerPrice.IsNil() || !o.TriggerPrice.IsZero())will also reject the impossible state “pointer not nil but value nil”.
You can simplify and improve clarity:-if o.TriggerPrice != nil && (o.TriggerPrice.IsNil() || !o.TriggerPrice.IsZero()) { +if o.TriggerPrice != nil && !o.TriggerPrice.IsZero() { return ErrInvalidTriggerPrice }
1395-1410: Inefficient address equality check
bytes.Equal(SubaccountIDToSdkAddress(sourceSubaccountID).Bytes(), senderAddr.Bytes())
allocates two temporary slices on every call.Since both are
sdk.AccAddress, simply doif !SubaccountIDToSdkAddress(sourceSubaccountID).Equals(senderAddr) { ... }chain/exchange/types/derivative_orders.go (1)
160-172:ResizeReduceOnlyOrdersilently ignores quantity increasesWhen
newQuantity ≥ oldQuantitythe function returnsnil, giving the
caller no indication that the resize was a no-op. This can hide logic
errors.Return an explicit error instead:
- if !quantityDecrement.IsPositive() { - return nil - } + if !quantityDecrement.IsPositive() { + return errors.Wrap(ErrOrderInvalid, "new quantity must be lower for reduce-only resize") + }
🛑 Comments failed to post (17)
chain/exchange/types/common_order.go (1)
10-10: 💡 Verification agent
🧩 Analysis chain
Verify protobuf library compatibility.
The import has been changed from
"github.com/cosmos/gogoproto/proto"to"github.com/gogo/protobuf/proto". Ensure this change maintains compatibility with the existing protobuf usage in the codebase.
🏁 Script executed:
#!/bin/bash # Check for consistent protobuf imports across the codebase echo "Checking for gogoproto imports:" rg "github.com/cosmos/gogoproto/proto" echo "Checking for gogo protobuf imports:" rg "github.com/gogo/protobuf/proto" echo "Checking proto.UnmarshalJSONEnum usage:" rg -A 2 -B 2 "proto\.UnmarshalJSONEnum"Length of output: 10838
Inconsistent Protobuf import in common_order.go
The rest of the codebase (including all generated
.pb.gofiles and the v2 common_order) imports"github.com/cosmos/gogoproto/proto"but here we’ve switched to
"github.com/gogo/protobuf/proto"This mismatch will lead to duplicate symbols and build failures. Please revert the import at
chain/exchange/types/common_order.go:10
back to:- "github.com/gogo/protobuf/proto" + "github.com/cosmos/gogoproto/proto"🤖 Prompt for AI Agents
In chain/exchange/types/common_order.go at line 10, the protobuf import was changed to "github.com/gogo/protobuf/proto" which is inconsistent with the rest of the codebase that uses "github.com/cosmos/gogoproto/proto". Revert the import statement back to "github.com/cosmos/gogoproto/proto" to maintain compatibility and avoid build conflicts.chain/evm/types/codec.go (3)
37-49:
⚠️ Potential issueDuplicate registration will panic at runtime
MsgEthereumTxis registered twice for the same interface (sdk.Msg).
InterfaceRegistry.RegisterImplementationspanics on duplicate registrations, so this will bring the node down during module init.-registry.RegisterImplementations( - (*sdk.Msg)(nil), - &MsgEthereumTx{}, -) ... -registry.RegisterImplementations( - (*sdk.Msg)(nil), - &MsgEthereumTx{}, - &MsgUpdateParams{}, -) +registry.RegisterImplementations( + (*sdk.Msg)(nil), + &MsgEthereumTx{}, + &MsgUpdateParams{}, +)📝 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.registry.RegisterImplementations( (*tx.TxExtensionOptionI)(nil), &ExtensionOptionsEthereumTx{}, ) registry.RegisterImplementations( (*sdk.Msg)(nil), &MsgEthereumTx{}, &MsgUpdateParams{}, )🤖 Prompt for AI Agents
In chain/evm/types/codec.go between lines 37 and 49, MsgEthereumTx is registered twice for the sdk.Msg interface, causing a runtime panic due to duplicate registration. Remove the redundant registration of MsgEthereumTx under sdk.Msg so that each implementation is registered only once per interface to prevent the node from crashing during module initialization.
80-88: 🛠️ Refactor suggestion
UnpackTxDatafails for freshly-decodedAny
any.GetCachedValue()returnsnilwhen theAnywas unmarshalled from bytes/JSON and not yet unpacked, so the type assertion will panic/return error in all normal code paths.Use the codec
AnyUnpackerinstead:- txData, ok := any.GetCachedValue().(TxData) - if !ok { - return nil, errorsmod.Wrapf(errortypes.ErrUnpackAny, "cannot unpack Any into TxData %T", any) - } + var txData TxData + if err := codectypes.UnpackAny(any, &txData); err != nil { + return nil, errorsmod.Wrap(errortypes.ErrUnpackAny, err.Error()) + }📝 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 UnpackTxData(any *codectypes.Any) (TxData, error) { if any == nil { return nil, errorsmod.Wrap(errortypes.ErrUnpackAny, "protobuf Any message cannot be nil") } var txData TxData if err := codectypes.UnpackAny(any, &txData); err != nil { return nil, errorsmod.Wrap(errortypes.ErrUnpackAny, err.Error()) }🤖 Prompt for AI Agents
In chain/evm/types/codec.go around lines 80 to 88, the UnpackTxData function incorrectly uses any.GetCachedValue() which returns nil for freshly decoded Any messages, causing the type assertion to fail. Replace this with the proper codec AnyUnpacker interface to unpack the Any message safely. Modify the function to accept an AnyUnpacker parameter and use its UnpackAny method to unpack the Any into a TxData instance, handling errors accordingly.
18-22: 🛠️ Refactor suggestion
ModuleCdcuses a fresh registry – interfaces are never registered
ModuleCdcis created with a brand-newInterfaceRegistry, butRegisterInterfacesis never invoked on that instance, therefore JSON/Proto marshalling viaModuleCdcwill fail to unpack Any types.Add the registrations during
init():ModuleCdc = codec.NewProtoCodec(codectypes.NewInterfaceRegistry()) ... func init() { RegisterLegacyAminoCodec(amino) + RegisterInterfaces(ModuleCdc.InterfaceRegistry) amino.Seal() }🤖 Prompt for AI Agents
In chain/evm/types/codec.go around lines 18 to 22, the ModuleCdc is initialized with a new InterfaceRegistry but the necessary interface registrations are missing, causing JSON/Proto marshalling to fail for Any types. Fix this by adding a call to RegisterInterfaces on the InterfaceRegistry instance used by ModuleCdc during the init() function to properly register all required interfaces.chain/evm/types/chain_config.go (2)
178-184: 🛠️ Refactor suggestion
ValidateHashdoes not actually validate a hashThe current check only rejects blank strings; malformed lengths or non-hex characters slip through.
func ValidateHash(hex string) error { - if hex != "" && strings.TrimSpace(hex) == "" { - return errorsmod.Wrap(ErrInvalidChainConfig, "hash cannot be blank") - } + if hex == "" { + return nil // empty is allowed + } + if !common.IsHexAddress(hex) && !hexRegex.MatchString(hex) { + return errorsmod.Wrap(ErrInvalidChainConfig, "invalid hex hash") + } + if len(strings.TrimPrefix(hex, "0x")) != 64 { + return errorsmod.Wrap(ErrInvalidChainConfig, "hash must be 32 bytes") + } 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 ValidateHash(hex string) error { if hex == "" { return nil // empty is allowed } if !common.IsHexAddress(hex) && !hexRegex.MatchString(hex) { return errorsmod.Wrap(ErrInvalidChainConfig, "invalid hex hash") } if len(strings.TrimPrefix(hex, "0x")) != 64 { return errorsmod.Wrap(ErrInvalidChainConfig, "hash must be 32 bytes") } return nil }🤖 Prompt for AI Agents
In chain/evm/types/chain_config.go around lines 178 to 184, the ValidateHash function only checks for blank strings but does not verify if the input is a valid hex string or if it has the correct length for a hash. Update the function to check that the string is a valid hexadecimal string and that its length matches the expected hash length (e.g., 64 characters for a 32-byte hash). Return an error if these conditions are not met.
104-110: 💡 Verification agent
🧩 Analysis chain
Potential uint64 overflow when converting fork timestamps
big.Int.Uint64()silently truncates values larger thanmath.MaxUint64.
Add an explicit upper-bound check to avoid subtle fork-time corruption.func getTimeValue(time *sdkmath.Int) *uint64 { if time == nil || time.IsNegative() { return nil } - t := time.BigIntMut().Uint64() + if time.BitLen() > 64 { + return nil // or return an error, but avoid silent overflow + } + t := time.BigIntMut().Uint64() return &t }
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the chain_config.go file FILE=$(fd chain_config.go || echo "") if [ -z "$FILE" ]; then echo "chain_config.go not found" >&2 exit 1 fi echo "=== File: $FILE ===" echo echo "---- Imports and package declaration (lines 1–30) ----" sed -n '1,30p' "$FILE" echo echo "---- getTimeValue definition (around lines 80–140) ----" grep -R -n "func getTimeValue" -n "$FILE" sed -n '80,140p' "$FILE"Length of output: 3669
Add an explicit overflow check in getTimeValue to prevent silent uint64 truncation
The current implementation will drop high-order bits if the timestamp exceeds math.MaxUint64. It’s safer to bail out before calling
Uint64():• File:
chain/evm/types/chain_config.go
Location: insidefunc getTimeValue(around line 104)Suggested diff:
func getTimeValue(time *sdkmath.Int) *uint64 { if time == nil || time.IsNegative() { return nil } - t := time.BigIntMut().Uint64() - return &t + // protect against values > 2^64-1, which Uint64() would silently truncate + bigInt := time.BigIntMut() + if bigInt.BitLen() > 64 { + return nil + } + t := bigInt.Uint64() + return &t }📝 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 getTimeValue(time *sdkmath.Int) *uint64 { if time == nil || time.IsNegative() { return nil } // protect against values > 2^64-1, which Uint64() would silently truncate bigInt := time.BigIntMut() if bigInt.BitLen() > 64 { return nil } t := bigInt.Uint64() return &t }🤖 Prompt for AI Agents
In chain/evm/types/chain_config.go around lines 104 to 110, add an explicit check before calling Uint64() on the big.Int to ensure the value does not exceed math.MaxUint64. If the value is greater, return nil or handle the overflow case appropriately to prevent silent truncation and potential fork-time corruption.chain/exchange/types/common_utils.go (1)
322-331: 🛠️ Refactor suggestion
Hex duplicate check silently tolerates malformed hashes
common.HexToHashconverts any non-hex or wrong-length string into the zero hash, causing false positives/negatives:common.HexToHash("foo") == common.HexToHash("") // both become 0-hashReject invalid hashes instead:
func HasDuplicatesHexHash(slice []string) bool { seen := make(map[common.Hash]struct{}) for _, item := range slice { - if _, ok := seen[common.HexToHash(item)]; ok { + if !IsHexHash(item) { // reuse existing helper + return true // or handle error upstream + } + h := common.HexToHash(item) + if _, ok := seen[h]; ok { return true } - seen[common.HexToHash(item)] = struct{}{} + seen[h] = struct{}{} } return false }🤖 Prompt for AI Agents
In chain/exchange/types/common_utils.go around lines 322 to 331, the function HasDuplicatesHexHash uses common.HexToHash which converts invalid hex strings into a zero hash, causing false positives in duplicate detection. Modify the function to first validate each string as a proper hex hash and reject or skip invalid entries instead of converting them, ensuring only valid hashes are checked for duplicates.chain/evm/types/params.go (1)
72-82: 🛠️ Refactor suggestion
Case-sensitivity & duplication checks for authorised deployers can fail silently.
ValidateAuthorizedDeployerstreats addresses that differ only in checksum case as distinct values, andIsAuthorisedDeployerlater does a case-sensitive lookup. Because Ethereum addresses are case-insensitive (EIP-55 checksum notwithstanding) this can lead to:
- Duplicate addresses slipping through validation if the checksum case differs.
- Legitimate deployers being rejected at runtime.
Consider normalising to lowercase (or checksum-encoded) strings both when validating and when looking up.
+import "strings" @@ - for _, addrStr := range v { + for _, addrStr := range v { + addr := strings.ToLower(addrStr) - if !common.IsHexAddress(addrStr) { + if !common.IsHexAddress(addr) { ... - if _, found := authorisedAddresses[addrStr]; found { + if _, found := authorisedAddresses[addr]; found { ... - authorisedAddresses[addrStr] = struct{}{} + authorisedAddresses[addr] = struct{}{} }and in
IsAuthorisedDeployer:-return slices.Contains(p.AuthorizedDeployers, addr.Hex()) +return slices.Contains(p.AuthorizedDeployers, strings.ToLower(addr.Hex()))🤖 Prompt for AI Agents
In chain/evm/types/params.go around lines 72 to 82, the validation and lookup of authorized deployer addresses are case-sensitive, causing duplicates and incorrect rejections. To fix this, normalize all authorized deployer addresses to lowercase (or a consistent checksum format) during validation in validateAuthorizedDeployers and also convert addresses to the same normalized form in IsAuthorisedDeployer before lookup. This ensures consistent, case-insensitive handling of Ethereum addresses.chain/evm/types/dynamic_fee_tx.go (2)
66-82: 🛠️ Refactor suggestion
Copy()performs a shallow copy for pointer fields – changes in the clone mutate the original
GasTipCap,GasFeeCap,Amount, andAccessesare copied by reference, so callers modifying the clone will inadvertently alter the source object.
Either:
- Document loudly that
Copy()is not a deep copy, or- (Recommended) deep-copy these fields:
return &DynamicFeeTx{ ChainID: tx.ChainID, - GasTipCap: tx.GasTipCap, - GasFeeCap: tx.GasFeeCap, - Amount: tx.Amount, - Accesses: tx.Accesses, + GasTipCap: sdkmath.NewIntFromBigInt(tx.GetGasTipCap()), // nil-safe helper + GasFeeCap: sdkmath.NewIntFromBigInt(tx.GetGasFeeCap()), + Amount: func() *sdkmath.Int { if tx.Amount==nil {return nil}; v := sdkmath.NewIntFromBigInt(tx.Amount.BigInt()); return &v }(), + Accesses: func() *AccessList { if tx.Accesses==nil {return nil}; al := tx.Accesses.Copy(); return &al }(),Failing to do so makes bugs very hard to trace.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In chain/evm/types/dynamic_fee_tx.go around lines 66 to 82, the Copy() method performs a shallow copy for pointer fields GasTipCap, GasFeeCap, Amount, and Accesses, causing modifications in the clone to affect the original. To fix this, update Copy() to deep-copy these fields by creating new instances or slices with the same values instead of copying references. This ensures changes to the clone do not mutate the original object.
16-59: 🛠️ Refactor suggestion
Slice aliasing risk – copy
tx.Data()to avoid unexpected mutations
tx.Data()returns the original slice stored inside the go-ethereumTransaction.
Whilst that slice is currently immutable, holding the same backing array in two packages is fragile (future library/internal modifications or test mocks may mutate the slice).
Safest is to deep-copy on assignment, mirroring what you already do inCopy()for theDatafield:- Data: tx.Data(), + Data: common.CopyBytes(tx.Data()),This keeps the invariants symmetrical and removes a subtle aliasing hazard.
📝 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 NewDynamicFeeTx(tx *ethtypes.Transaction) (*DynamicFeeTx, error) { txData := &DynamicFeeTx{ Nonce: tx.Nonce(), Data: common.CopyBytes(tx.Data()), GasLimit: tx.Gas(), } v, r, s := tx.RawSignatureValues() if to := tx.To(); to != nil { txData.To = to.Hex() } if tx.Value() != nil { amountInt, err := types.SafeNewIntFromBigInt(tx.Value()) if err != nil { return nil, err } txData.Amount = &amountInt } if tx.GasFeeCap() != nil { gasFeeCapInt, err := types.SafeNewIntFromBigInt(tx.GasFeeCap()) if err != nil { return nil, err } txData.GasFeeCap = &gasFeeCapInt } if tx.GasTipCap() != nil { gasTipCapInt, err := types.SafeNewIntFromBigInt(tx.GasTipCap()) if err != nil { return nil, err } txData.GasTipCap = &gasTipCapInt } if tx.AccessList() != nil { al := tx.AccessList() txData.Accesses = NewAccessList(&al) } txData.SetSignatureValues(tx.ChainId(), v, r, s) return txData, nil }🤖 Prompt for AI Agents
In chain/evm/types/dynamic_fee_tx.go around lines 16 to 59, the assignment of tx.Data() to the Data field directly aliases the original slice, risking unexpected mutations. To fix this, create a deep copy of the slice returned by tx.Data() before assigning it to the Data field, ensuring the internal state is independent and immutable, consistent with the approach used in the Copy() method.chain/evm/types/utils.go (1)
186-204:
⚠️ Potential issue
UnwrapEthereumMsgaborts early on the first non-EVM messageThe loop returns an error as soon as it encounters a non-
*MsgEthereumTx, meaning mixed-msg transactions (e.g. fee-grant + eth-tx) will always fail even if the desired Ethereum msg is present later.Replace the early-exit with a
continueso the search covers all messages:- ethMsg, ok := msg.(*MsgEthereumTx) - if !ok { - return nil, fmt.Errorf("invalid tx type: %T", tx) - } + ethMsg, ok := msg.(*MsgEthereumTx) + if !ok { + continue + }This makes the helper robust to multisig or decorator messages.
🤖 Prompt for AI Agents
In chain/evm/types/utils.go lines 186 to 204, the UnwrapEthereumMsg function incorrectly returns an error immediately upon encountering a non-*MsgEthereumTx message, causing it to fail on transactions with mixed message types. Modify the code to replace the early return on non-*MsgEthereumTx messages with a continue statement so the loop skips those messages and continues searching through all messages for the matching Ethereum transaction. This ensures the function correctly finds the Ethereum message even in mixed-message transactions.chain/evm/types/msg.go (3)
178-181: 🛠️ Refactor suggestion
Avoid hard-error in
GetMsgsV2Down-stream code (e.g. tx signing helpers) may optimistically call
GetMsgsV2. Returning an error bricks the flow even when ADR-051 is not required. Prefer returning(nil, nil)until full V2 support is implemented.-func (msg *MsgEthereumTx) GetMsgsV2() ([]proto.Message, error) { - return nil, errors.New("not implemented") -} +func (msg *MsgEthereumTx) GetMsgsV2() ([]proto.Message, error) { + // MsgEthereumTx does not yet support ADR-051 – return nil to indicate absence + return nil, 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 (msg *MsgEthereumTx) GetMsgsV2() ([]proto.Message, error) { // MsgEthereumTx does not yet support ADR-051 – return nil to indicate absence return nil, nil }🤖 Prompt for AI Agents
In chain/evm/types/msg.go around lines 178 to 181, the GetMsgsV2 method currently returns an error indicating it is not implemented, which disrupts downstream processes that may call it optimistically. Modify the method to return (nil, nil) instead of an error to allow the flow to continue smoothly until full V2 support is added.
142-149:
⚠️ Potential issueAdd length validation for
Fromto avoid malformed addresses
ValidateBasiconly checks thatmsg.Fromis non-empty. A slice with fewer (or more) thancommon.AddressLength == 20bytes is silently accepted and later converted withcommon.BytesToAddress, which truncates/zero-pads and may lead to funds being sent to the wrong address.@@ - if len(msg.From) == 0 { + if len(msg.From) == 0 { return errorsmod.Wrapf(errortypes.ErrInvalidRequest, "sender address is missing") } + if len(msg.From) != common.AddressLength { + return errorsmod.Wrapf( + errortypes.ErrInvalidAddress, + "sender address must be %d bytes, got %d", + common.AddressLength, + len(msg.From), + ) + }📝 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 (msg MsgEthereumTx) ValidateBasic() error { // From and Raw are only two fields allowed in new transaction format. if len(msg.From) == 0 { return errorsmod.Wrapf(errortypes.ErrInvalidRequest, "sender address is missing") } if len(msg.From) != common.AddressLength { return errorsmod.Wrapf( errortypes.ErrInvalidAddress, "sender address must be %d bytes, got %d", common.AddressLength, len(msg.From), ) } if msg.Raw.Transaction == nil { return errorsmod.Wrapf(errortypes.ErrInvalidRequest, "raw tx is missing") } // ... rest of ValidateBasic }🤖 Prompt for AI Agents
In chain/evm/types/msg.go around lines 142 to 149, the ValidateBasic method only checks if msg.From is non-empty but does not validate its length. Add a check to ensure that the length of msg.From is exactly common.AddressLength (20 bytes). If the length is incorrect, return an error indicating the sender address is malformed. This prevents incorrect address conversions that could cause funds to be sent to the wrong address.
251-255:
⚠️ Potential issueHandle EIP-1559 transactions in
GetFeeFor dynamic-fee txs (
GasPrice() == nil) the current logic panics onprice.Mul. Safely treat a nil/zero price as zero fee.-func (msg MsgEthereumTx) GetFee() *big.Int { - tx := msg.AsTransaction() - price := tx.GasPrice() - return price.Mul(price, new(big.Int).SetUint64(tx.Gas())) +func (msg MsgEthereumTx) GetFee() *big.Int { + tx := msg.AsTransaction() + price := tx.GasPrice() + if price == nil || price.Sign() == 0 { + return big.NewInt(0) + } + return new(big.Int).Mul(price, new(big.Int).SetUint64(tx.Gas())) }📝 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 (msg MsgEthereumTx) GetFee() *big.Int { tx := msg.AsTransaction() price := tx.GasPrice() if price == nil || price.Sign() == 0 { return big.NewInt(0) } return new(big.Int).Mul(price, new(big.Int).SetUint64(tx.Gas())) }🤖 Prompt for AI Agents
In chain/evm/types/msg.go around lines 251 to 255, the GetFee method does not handle EIP-1559 dynamic-fee transactions where GasPrice() can be nil, causing a panic on price.Mul. Modify the method to check if price is nil; if so, return zero as the fee. Otherwise, calculate the fee by multiplying price by gas as before.chain/exchange/types/market.go (1)
64-71: 🛠️ Refactor suggestion
Possible overflow / precision loss when doing
MulbeforeQuo
PriceFromChainFormatdoesreturn price.Mul(baseMultiplier).Quo(quoteMultiplier)For large
pricevalues (e.g. close toMaxDec) and a high-precision market (both multipliers up to1e18) the intermediate product can overflowmath.LegacyDec’s internalbig.Int, or lose precision that would not be lost if the division was executed first.- return price.Mul(baseMultiplier).Quo(quoteMultiplier) + // divide first to keep the intermediate number as small as possible + return price.Quo(quoteMultiplier).Mul(baseMultiplier)Same observation applies to
PriceToChainFormat,Quantity/Notional*FromChainFormat/ToChainFormatbelow.🤖 Prompt for AI Agents
In chain/exchange/types/market.go around lines 64 to 71, the function PriceFromChainFormat multiplies price by baseMultiplier before dividing by quoteMultiplier, which can cause overflow or precision loss for large values. To fix this, reorder the operations to perform the division first (price.Quo(quoteMultiplier)) followed by the multiplication (Mul(baseMultiplier)) to minimize intermediate value size and preserve precision. Apply the same change to PriceToChainFormat and the Quantity/Notional FromChainFormat/ToChainFormat functions to ensure consistent handling of large values.chain/exchange/types/msgs.go (1)
238-242:
⚠️ Potential issueMargin-ratio relation check breaks when only one ratio is provided
ValidateBasic()now runsif msg.NewInitialMarginRatio.LTE(msg.NewMaintenanceMarginRatio) { return ErrMarginsRelation }inside a block that is executed when either ratio is updated.
If the user updates only the maintenance ratio,NewInitialMarginRatio
is zero, so the check always fails even though initial ratio is
unchanged on-chain.Guard the comparison with a “both updated” condition:
-if msg.NewInitialMarginRatio.LTE(msg.NewMaintenanceMarginRatio) { +if msg.HasInitialMarginRatioUpdate() && msg.HasMaintenanceMarginRatioUpdate() && + msg.NewInitialMarginRatio.LTE(msg.NewMaintenanceMarginRatio) { return ErrMarginsRelation }🤖 Prompt for AI Agents
In chain/exchange/types/msgs.go around lines 238 to 242, the margin ratio relation check incorrectly runs when only one of the ratios is updated, causing false failures if only the maintenance ratio is updated. Modify the condition to ensure the comparison between NewInitialMarginRatio and NewMaintenanceMarginRatio only occurs if both ratios are updated, guarding the check with a condition that confirms both updates are present before performing the LTE comparison.chain/exchange/types/derivative_orders.go (1)
84-86:
⚠️ Potential issue
ComputeOrderHashmutates the order in-placeThe test helper rewrites
m.OrderInfo.Priceandm.Marginbefore
computing the hash:m.OrderInfo.Price = ... m.Margin = ...This side-effect can leak into subsequent test logic and produce
hard-to-trace failures.Clone the order or keep the converted values in locals:
- m.OrderInfo.Price = PriceFromChainFormat(...) - m.Margin = NotionalFromChainFormat(...) + price := PriceFromChainFormat(m.OrderInfo.Price, 0, scaleFactor) + margin := NotionalFromChainFormat(m.Margin, scaleFactor)and use the locals in the
ComputeOrderHashcall.🤖 Prompt for AI Agents
In chain/exchange/types/derivative_orders.go around lines 84 to 86, the code mutates the order in-place by assigning converted values directly to m.OrderInfo.Price and m.Margin before computing the hash. To fix this, avoid mutating the original order by cloning the order or storing the converted Price and Margin values in local variables, then use these locals when calling ComputeOrderHash. This prevents side effects that could affect subsequent test logic.
devand the latestmasterbranchesSummary by CodeRabbit