-
Notifications
You must be signed in to change notification settings - Fork 60
refactor: replace interface{} with any for clarity and modernization #341
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: weifangc <weifangcheng@outlook.jp>
WalkthroughThe changes replace Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
chain/peggy/types/params.go (1)
183-194: Avoid error‑string matching invalidateBridgeContractAddressThe special‑casing of empty addresses by checking
strings.Contains(err.Error(), "empty")couples behavior to the error message text fromValidateEthAddress, which is brittle and inconsistent withvalidateCosmosCoinErc20Contract(which just checksv == "").Consider simplifying to an explicit empty‑string guard and delegating to
ValidateEthAddressotherwise:-func validateBridgeContractAddress(i any) error { - v, ok := i.(string) - if !ok { - return fmt.Errorf("invalid parameter type: %T", i) - } - if err := ValidateEthAddress(v); err != nil { - if !strings.Contains(err.Error(), "empty") { - return err - } - } - return nil -} +func validateBridgeContractAddress(i any) error { + v, ok := i.(string) + if !ok { + return fmt.Errorf("invalid parameter type: %T", i) + } + if v == "" { + // empty address is allowed + return nil + } + return ValidateEthAddress(v) +}This is clearer and robust to future changes in
ValidateEthAddress’s error text.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
chain/evm/types/errors.go(1 hunks)chain/evm/types/params.go(3 hunks)chain/exchange/types/common_order.go(1 hunks)chain/exchange/types/params.go(21 hunks)chain/exchange/types/paramset.go(1 hunks)chain/exchange/types/spot_orders.go(1 hunks)chain/oracle/types/params.go(1 hunks)chain/peggy/types/params.go(10 hunks)chain/wasmx/types/custom_execution.go(1 hunks)chain/wasmx/types/params.go(5 hunks)client/chain/chain.go(2 hunks)client/chain/chain_v2.go(2 hunks)eip712_cosmos.go(3 hunks)ethereum/util/amounts.go(1 hunks)ethereum/util/contract.go(1 hunks)examples/chain/11_DecodeTx/37_decode_tx.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
chain/evm/types/params.go (1)
chain/exchange/types/params.go (1)
ValidateBool(618-624)
chain/exchange/types/params.go (2)
chain/exchange/types/v2/params.go (1)
ValidateAtomicMarketOrderAccessLevel(274-283)chain/evm/types/params.go (1)
ValidateBool(125-131)
🔇 Additional comments (16)
chain/oracle/types/params.go (1)
88-103: Use ofanyinvalidatePythContractis safe and consistentSwitching the validator arg to
anyis a no-op semantically and keeps compatibility with the params framework.chain/wasmx/types/custom_execution.go (1)
15-19:ExecutionData.Argschange toanyis benign
Argsremains an unconstrained JSON payload; usinganyis idiomatic and does not alter marshaling or call sites.client/chain/chain_v2.go (1)
2741-2782: EIP-712message/OrderInfomaps now usemap[string]any– no behavioral changeThe switch to
map[string]anyin both spot and derivative branches preserves the TypedData structure and hashing; EIP-712 order hash computation remains identical.Also applies to: 2784-2825
client/chain/chain.go (1)
2925-2966: Consistentanyusage in ComputeOrderHashes mapsUpdating the spot and derivative TypedData
message/OrderInfomaps tomap[string]anykeeps the EIP‑712 payload shape identical and just modernizes the type spelling.Also applies to: 2968-3009
examples/chain/11_DecodeTx/37_decode_tx.go (1)
11-14:parserMaprefactor tomap[string]anyis fineThe map still holds concrete message prototypes and the type switch logic is unaffected.
chain/exchange/types/common_order.go (1)
132-144: Adoptinganyin EIP‑712 order message andGetOrderIdentifierUsing
map[string]anyfor the EIP‑712message/OrderInfoand returninganyfromGetOrderIdentifierpreserves existing behavior while aligning with modern Go style.Also applies to: 198-204
ethereum/util/amounts.go (1)
25-46:Wei.Scannow takesany– implementation and interface compatibility intactThe Scan logic is unchanged and still matches scanner expectations; this is a purely cosmetic alias change.
chain/evm/types/errors.go (1)
295-297:RevertError.ErrorDatareturn type switched toanywith no behavior changeError data remains the hex string revert reason; this just updates the return type spelling to
any.eip712_cosmos.go (1)
122-143:map[string]anyrefactor here is behavior‑preservingUsing
anyas an alias forinterface{}intxDataandctxkeeps JSON (un)marshalling and thefeetype assertion semantics unchanged, so this is a safe readability/modernization tweak. Just ensure the module’sgoversion is 1.18+ ingo.modsoanyis available.Also applies to: 213-220
ethereum/util/contract.go (1)
90-99: Transact...anysignature is source‑compatibleSwitching the variadic parameter to
...anyis type‑identical to...interface{}and keeps both the directBoundContract.Transactcall and the ABI packing path behaving the same, while modernizing the API surface. Just confirm the repo targets Go 1.18+.chain/exchange/types/paramset.go (1)
4-18: ParamSet APIs updated toanywith no semantic changeUsing
anyforValueValidatorFn,ParamSetPair.Value, andNewParamSetPairkeeps behavior identical while aligning with modern Go style. Public API remains type‑compatible sinceanyis just an alias forinterface{}.chain/peggy/types/params.go (1)
153-181: New minimums for time‑related params change validation behavior
validateTargetBatchTimeout,validateAverageBlockTime, andvalidateAverageEthereumBlockTimenow enforce lower bounds (>= 60000and>= 100respectively). That will cause param sets or proposals with smaller values that previously passedValidateBasicto start failing. Given the PR is described as a refactor toany, please double‑check that tightening these constraints in this PR (and at this magnitude) is intentional and acceptable for existing deployments.chain/evm/types/params.go (1)
116-153: EVM param validators now enforce stricter semanticsBeyond the
anyrefactor,validateEIPsnow rejects non‑activateable EIPs viavm.ValidEip, andValidateChainConfigdelegates tocfg.Validate(). That’s a behavioral tightening: someExtraEIPsorChainConfigvalues that previously passed will now causeParams.Validate()to error. Please confirm that this stricter validation is desired in this PR (vs. being split into a separate, explicitly behavior‑changing change).chain/wasmx/types/params.go (1)
105-149: New gas constraints materially tighten wasmx parameter validation
validateMaxBeginBlockTotalGas,validateMaxContractGasLimit, andvalidateMinGasPricenow enforce non‑zero and minimum‑limit checks (includingMaxContractGasLimit >= MinExecutionGasLimit). This is beneficial from a safety perspective but will causeParams.Validate()and param proposals with 0 or very low values that previously passed to start failing.Since the PR is framed as a type‑alias refactor, please confirm that introducing these new constraints here is intentional and that no existing networks rely on
0or sub‑MinExecutionGasLimitvalues as sentinels.chain/exchange/types/spot_orders.go (1)
33-44: LGTM! Map type declarations modernized consistently.The map type declarations have been updated from
map[string]interface{}tomap[string]anyfor both the top-level message and the nested OrderInfo payload. This is consistent with the broader refactoring effort and improves code readability without any functional changes.chain/exchange/types/params.go (1)
241-624: Refactoring approved—Go version supportsanyalias.The project requires Go 1.23.9, which exceeds the minimum 1.18 requirement for the
anytype alias. All validator function signature updates are safe and idiomatic.
This change replaces occurrences of interface{} with the predeclared identifier any, introduced in Go 1.18 as an alias for interface{}.
As noted in the Go 1.18 Release Notes:
This improves readability and aligns the codebase with modern Go conventions.
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.