added abi for Kyber, Odos, 1INCH aggregators and wildcard target rule#70
added abi for Kyber, Odos, 1INCH aggregators and wildcard target rule#70evgensheff wants to merge 8 commits intomainfrom
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (6)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds three new ABI JSONs (OdosRouterV2, RouterV5_1inch, kyber_RouterV2), generates Go bindings for OdosRouterV2 and KyberRouterV2, introduces TARGET_TYPE_WILDCARD in proto/rule.proto, prints the abigen command in the generator, and allows wildcard targets for "approve" in engine/evm/evm.go. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant OdosRouterV2
note over App,OdosRouterV2: New OdosRouterV2 swap flows (single/multi, permit2, router-funds)
User->>App: Request swap
App->>OdosRouterV2: swap/swapMulti(…pathDefinition…, executor, referralCode)
alt Permit2
App->>OdosRouterV2: swapPermit2/swapMultiPermit2(permit2,…)
end
OdosRouterV2-->>App: amountOut / amountsOut
OdosRouterV2-->>App: Swap / SwapMulti event
App-->>User: Return result
sequenceDiagram
autonumber
actor User
participant App
participant KyberRouterV2
note over App,KyberRouterV2: Kyber MetaAggregationRouterV2 aggregated swaps
User->>App: Execute aggregated swap
App->>KyberRouterV2: swap/swapGeneric/swapSimpleMode(execution)
KyberRouterV2-->>App: returnAmount, gasUsed
KyberRouterV2-->>App: Swapped/Exchange/Fee events
App-->>User: Deliver output
sequenceDiagram
autonumber
actor User
participant App
participant RouterV5_1inch
note over App,RouterV5_1inch: 1inch Router V5 order & RFQ flows
User->>App: Fill order / RFQ
App->>RouterV5_1inch: fillOrder*/fillOrderRFQ*/unoswap*/uniswapV3Swap*(…)
RouterV5_1inch-->>App: returnAmount, spentAmount
RouterV5_1inch-->>App: OrderFilled / OrderFilledRFQ events
App-->>User: Summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/gen_abi_bind/main.go (1)
34-47: Skip non-JSON entries and directories to avoid accidental abigen failures.Currently every entry is processed; filter to regular
.jsonfiles.for _, entry := range list { - noExt := strings.TrimSuffix(entry.Name(), ".json") + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".json") { + continue + } + noExt := strings.TrimSuffix(entry.Name(), ".json") outSubfolder := path.Join(outPath, strings.ToLower(noExt))
🧹 Nitpick comments (6)
proto/rule.proto (2)
37-42: Document semantics and enforce enum/oneof consistency for WILDCARD.Add brief docs explaining how wildcard matching works and ensure validation enforces that
target_type == TARGET_TYPE_WILDCARDimplies thewildcardfield is set (and vice versa). This avoids ambiguous Target payloads at call sites.enum TargetType { TARGET_TYPE_UNSPECIFIED = 0; TARGET_TYPE_ADDRESS = 1; TARGET_TYPE_MAGIC_CONSTANT = 2; + // Matches multiple targets via pattern rules (e.g. "*", "chain:1:*", "protocol:1inch"). + // When using this type, the oneof field `wildcard` must be set with a non-empty pattern. TARGET_TYPE_WILDCARD = 3; }
44-51: Add field-level docs forwildcardand specify allowed patterns.Clarify supported syntax (glob, prefix, exact tokens), case-sensitivity, and whether checks are performed against addresses, function selectors, or protocol identifiers.
message Target { TargetType target_type = 1; oneof target { string address = 2; MagicConstant magic_constant = 3; - string wildcard = 4; + // Pattern used when target_type == TARGET_TYPE_WILDCARD. + // Examples: "*", "chain:1:*", "router:kyber", "address:0xabc...". + // Must be non-empty; matching rules should be documented alongside the evaluator. + string wildcard = 4; } }sdk/evm/codegen/odosrouterv2/odosrouterv2.go (4)
27-58: Type names violate Go export casing; consider generator tweak.Rename to OdosRouterV2InputTokenInfo, OdosRouterV2OutputTokenInfo, OdosRouterV2Permit2Info, OdosRouterV2SwapTokenInfo for idiomatic Go and to reduce lint noise.
-type OdosRouterV2inputTokenInfo struct { +type OdosRouterV2InputTokenInfo struct { @@ -type OdosRouterV2outputTokenInfo struct { +type OdosRouterV2OutputTokenInfo struct { @@ -type OdosRouterV2permit2Info struct { +type OdosRouterV2Permit2Info struct { @@ -type OdosRouterV2swapTokenInfo struct { +type OdosRouterV2SwapTokenInfo struct {Note: This requires adjusting all references in this file and call sites or updating the codegen to emit proper casing.
338-371: Tuple type alias should match renamed struct if you adopt idiomatic casing.If you rename OdosRouterV2swapTokenInfo → OdosRouterV2SwapTokenInfo, update Pack/TryPack signatures accordingly.
-func (odosrouterv2 *Odosrouterv2) PackSwap(tokenInfo OdosRouterV2swapTokenInfo, pathDefinition []byte, executor common.Address, referralCode uint32) []byte { +func (odosrouterv2 *Odosrouterv2) PackSwap(tokenInfo OdosRouterV2SwapTokenInfo, pathDefinition []byte, executor common.Address, referralCode uint32) []byte {
80-85: BoundContract helper is useful; consider adding typed helpers later.Instance wrapper is fine. If desired, future generator enhancement could add typed Call/Transact helpers for common methods to reduce manual ABI packing in call sites.
60-65: Preserve internalType whitespace in embedded ABI
The embedded ABI matchesabi/OdosRouterV2.jsonexcept for missing spaces ininternalTypevalues (e.g."structOdosRouterV2.swapTokenInfo"vs"struct OdosRouterV2.swapTokenInfo"). This mutation may break tools or equality checks that rely on the original JSON. Update the generator to embed the ABI verbatim or reinsert the space after"struct".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
types/constraint.pb.gois excluded by!**/*.pb.gotypes/parameter_constraint.pb.gois excluded by!**/*.pb.gotypes/policy.pb.gois excluded by!**/*.pb.gotypes/recipe_specification.pb.gois excluded by!**/*.pb.gotypes/resource.pb.gois excluded by!**/*.pb.gotypes/rule.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
abi/OdosRouterV2.json(1 hunks)abi/RouterV5_1inch.json(1 hunks)abi/kyber_RouterV2.json(1 hunks)cmd/gen_abi_bind/main.go(1 hunks)proto/rule.proto(1 hunks)sdk/evm/codegen/kyber_routerv2/kyber_routerv2.go(1 hunks)sdk/evm/codegen/odosrouterv2/odosrouterv2.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
sdk/evm/codegen/kyber_routerv2/kyber_routerv2.go (1)
sdk/evm/codegen/routerv5_1inch/routerv5_1inch.go (1)
SwapOutput(1297-1300)
sdk/evm/codegen/odosrouterv2/odosrouterv2.go (1)
ethereum/abi.go (1)
ParseABI(43-61)
🔇 Additional comments (11)
proto/rule.proto (1)
33-35: Commit regenerated Go protobuf and auditTargetconsumers
Ensuretypes/rule.pb.goincludes the newTARGET_TYPE_WILDCARDand review all callsites ofTargetto handle theTarget_Wildcardcase instead of defaulting to UNSPECIFIED.abi/RouterV5_1inch.json (1)
513-543: Overload binding suffixes verified Confirmed abigen v2 generated PackCancelOrderRFQ/TryPackCancelOrderRFQ and PackCancelOrderRFQ0/TryPackCancelOrderRFQ0 as expected; no further changes required.abi/kyber_RouterV2.json (1)
1-586: LGTM: ABI shape is consistent with bindings.Constructor, events, and swap entry points align; no structural issues spotted.
sdk/evm/codegen/kyber_routerv2/kyber_routerv2.go (3)
1-75: LGTM: Generated binding is coherent with ABI; constructors and Instance wrapper are correct.
238-281: Panic-on-pack is acceptable for generated API; TryPack alternatives are present.No action needed; just calling out the intended usage split.
427-453: Event unpackers look correct; topic checks and abi.UnpackIntoInterface are in place.No correctness issues detected.
Also applies to: 472-494, 515-537, 560-583, 603-625, 649-671
cmd/gen_abi_bind/main.go (1)
49-53: Nice: printing the abigen command aids reproducibility.The added debug print is helpful for CI logs and local troubleshooting.
abi/OdosRouterV2.json (2)
1-765: ABI surface looks consistent and ready to consume.Constructor, ownership, referral, swap variants (single/multi, Permit2, router-funds), and receive() are coherently modeled; mutability and tuple shapes align.
320-331: Confirm intent behind compact swap variants with zero inputs.swapCompact and swapMultiCompact expose payable functions with no inputs. If not used by your flows, consider omitting to minimize attack surface; otherwise document expected calldata/value semantics in the SDK.
Also applies to: 410-422
sdk/evm/codegen/odosrouterv2/odosrouterv2.go (2)
71-79: Constructor panic on invalid ABI is acceptable for generated code.Keeping panic here is fine; TryPack methods provide non-panicking paths elsewhere.
248-270: uint64 conversion assumes contract type width.ABI declares referralFee as uint64; Unpack uses ConvertType to uint64, which is correct. Just flagging that widening to big.Int would be safer if the contract ever widens this field.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/evm/evm.go (1)
220-224: Verify method selector and length before unpacking ABI args.Currently we slice
data[dataOffset:]without checking length and don’t confirm the first 4 bytes match the selected method. This can panic on short calldata and is a security footgun (false positives if arg shapes align). Add a length check and selector match.const dataOffset = 4 -args, err := method.Inputs.Unpack(data[dataOffset:]) +if len(data) < dataOffset { + return fmt.Errorf("failed to unpack abi args: calldata too short: len=%d", len(data)) +} +selector := data[:dataOffset] +expected := crypto.Keccak256([]byte(method.Sig))[:dataOffset] +if !bytes.Equal(selector, expected) { + return fmt.Errorf("failed to unpack abi args: method selector mismatch: expected=%x, actual=%x", expected, selector) +} +args, err := method.Inputs.Unpack(data[dataOffset:]) if err != nil { return fmt.Errorf("failed to unpack abi args: %w", err) }
🧹 Nitpick comments (5)
engine/evm/evm.go (5)
158-165: Wildcard target: allow-list looks good; add a guard against contract creation.Approving the scoped wildcard to only “approve”. Also ensure
to != nilso a contract-creation tx can’t slip past target checks.case types.TargetType_TARGET_TYPE_WILDCARD: if resource.FunctionId != "approve" { return fmt.Errorf( "tx target is wrong: wildcard only available for: approve, have: %s", resource.FunctionId, ) } + if to == nil { + return fmt.Errorf("tx target is wrong: contract creation not allowed for wildcard approve") + } return nil
3-20: Import crypto for the selector check.import ( "bytes" "fmt" "math/big" "path" "strings" "github.com/ethereum/go-ethereum/accounts/abi" "github.com/ethereum/go-ethereum/common" etypes "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/crypto" abi_embed "github.com/vultisig/recipes/abi" stdcompare "github.com/vultisig/recipes/engine/compare" "github.com/vultisig/recipes/engine/evm/compare" "github.com/vultisig/recipes/ethereum" "github.com/vultisig/recipes/resolver" "github.com/vultisig/recipes/types" "github.com/vultisig/recipes/util" )
126-132: Bubble up resolver error context.Include the underlying error to aid debugging.
- if err != nil { - return fmt.Errorf( - "failed to get resolver: magic_const=%s", - target.GetMagicConstant().String(), - ) - } + if err != nil { + return fmt.Errorf("failed to get resolver: magic_const=%s, err=%w", + target.GetMagicConstant().String(), err) + }
236-325: ABI type coverage may be insufficient for new aggregator ABIs.Odos/Kyber/1inch functions often use tuples, dynamic bytes, arrays of structs, etc. The current switch only supports a few scalars and address slices; others will hit “unsupported arg type”.
I can help extend this with additional comparers (e.g., bytes, bytes32[], uint256[], tuples via field-wise checks) or add an escape hatch to treat unnamed/complex params as CONSTRAINT_TYPE_ANY.
68-74: Non-zero msg.value for non-native contracts.Some aggregator swap methods are payable and expect ETH via
msg.value. The current hard fail on any non-native non-zero value will reject those. Confirm expected behavior; otherwise consider allowing value when the ABI method is payable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
types/constraint.pb.gois excluded by!**/*.pb.gotypes/parameter_constraint.pb.gois excluded by!**/*.pb.gotypes/policy.pb.gois excluded by!**/*.pb.gotypes/recipe_specification.pb.gois excluded by!**/*.pb.gotypes/resource.pb.gois excluded by!**/*.pb.gotypes/rule.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (1)
engine/evm/evm.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
engine/evm/evm.go (1)
types/rule.pb.go (1)
TargetType_TARGET_TYPE_WILDCARD(30-30)
|
Pausing this until the general chain integration is done |
2 participants
Summary by CodeRabbit
New Features
Chores