Skip to content
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

feat(solver): check target before accept #2478

Merged
merged 1 commit into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions e2e/solve/devapp/target.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"github.com/omni-network/omni/contracts/bindings"
"github.com/omni-network/omni/lib/errors"
"github.com/omni-network/omni/lib/evmchain"
solver "github.com/omni-network/omni/solver/app"
solver "github.com/omni-network/omni/solver/types"

"github.com/ethereum/go-ethereum/common"
)
Expand All @@ -26,7 +26,7 @@ func (t App) Address() common.Address {
return t.L1Vault
}

func (t App) IsAllowedCall(call *bindings.SolveCall) bool {
func (t App) IsAllowedCall(call bindings.SolveCall) bool {
if call.DestChainId != t.ChainID() {
return false
}
Expand All @@ -40,7 +40,7 @@ func (t App) IsAllowedCall(call *bindings.SolveCall) bool {
return err == nil
}

func (t App) TokenPrereqs(call *bindings.SolveCall) ([]*bindings.SolveTokenPrereq, error) {
func (t App) TokenPrereqs(call bindings.SolveCall) ([]bindings.SolveTokenPrereq, error) {
if !t.IsAllowedCall(call) {
return nil, errors.New("call not allowed")
}
Expand All @@ -50,15 +50,15 @@ func (t App) TokenPrereqs(call *bindings.SolveCall) ([]*bindings.SolveTokenPrere
return nil, errors.Wrap(err, "unpack deposit")
}

return []*bindings.SolveTokenPrereq{
return []bindings.SolveTokenPrereq{
{
Token: t.L1Token,
Amount: args.Amount,
},
}, nil
}

func (t App) Verify(srcChainID uint64, call *bindings.SolveCall, deposits []*bindings.SolveDeposit) error {
func (t App) Verify(srcChainID uint64, call bindings.SolveCall, deposits []bindings.SolveDeposit) error {
// we only accept deposits from mock L2
if srcChainID != evmchain.IDMockL2 {
return errors.New("source chain not supported", "src", srcChainID)
Expand All @@ -77,7 +77,7 @@ func (t App) Verify(srcChainID uint64, call *bindings.SolveCall, deposits []*bin

for _, deposit := range deposits {
if deposit.Token == t.L2Token {
l2token = deposit
l2token = &deposit
}
}

Expand Down
2 changes: 1 addition & 1 deletion solver/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ func startEventStreams(
ParseID: newIDParser(inboxContracts),
GetRequest: newRequestGetter(inboxContracts),
ShouldReject: newRequestValidator(def),
Accept: newAcceptor(inboxContracts, backends, solverAddr),
Accept: newAcceptor(network.ID, inboxContracts, backends, solverAddr),
Reject: newRejector(inboxContracts, backends, solverAddr),
Fulfill: newFulfiller(outboxContracts, backends, solverAddr),
Claim: newClaimer(inboxContracts, backends, solverAddr),
Expand Down
13 changes: 13 additions & 0 deletions solver/app/deps.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/omni-network/omni/lib/errors"
"github.com/omni-network/omni/lib/ethclient/ethbackend"
"github.com/omni-network/omni/lib/log"
"github.com/omni-network/omni/lib/netconf"

"github.com/ethereum/go-ethereum/accounts/abi/bind"
"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -136,11 +137,23 @@ func newRejector(
}

func newAcceptor(
network netconf.ID,
inboxContracts map[uint64]*bindings.SolveInbox,
backends ethbackend.Backends,
solverAddr common.Address,
) func(ctx context.Context, chainID uint64, req bindings.SolveRequest) error {
return func(ctx context.Context, chainID uint64, req bindings.SolveRequest) error {
target, err := targetFor(network, req.Call)
if err != nil {
log.Debug(ctx, "No target found for call", "call", req.Call)
return errors.Wrap(err, "get target")
}

if err := target.Verify(chainID, req.Call, req.Deposits); err != nil {
log.Debug(ctx, "Call rejected by target", "call", req.Call, "err", err)
return errors.Wrap(err, "verify target")
}

inbox, ok := inboxContracts[chainID]
if !ok {
return errors.New("unknown chain")
Expand Down
25 changes: 0 additions & 25 deletions solver/app/target.go

This file was deleted.

25 changes: 25 additions & 0 deletions solver/app/targets.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//nolint:unused // Some functions are unused but are kept for future use
package app

import (
"github.com/omni-network/omni/contracts/bindings"
"github.com/omni-network/omni/e2e/solve/devapp"
"github.com/omni-network/omni/lib/errors"
"github.com/omni-network/omni/lib/netconf"
"github.com/omni-network/omni/solver/types"
)

var (
targets map[netconf.ID]types.Targets = map[netconf.ID]types.Targets{
netconf.Devnet: {devapp.GetApp()},
}
)

func targetFor(network netconf.ID, call bindings.SolveCall) (types.Target, error) {
t, ok := targets[network]
if !ok {
return nil, errors.New("no targets for network", "network", network)
}

return t.ForCall(call)
}
50 changes: 50 additions & 0 deletions solver/types/target.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package types

import (
"github.com/omni-network/omni/contracts/bindings"
"github.com/omni-network/omni/lib/errors"

"github.com/ethereum/go-ethereum/common"
)

// Target is the interface for a target contract the solver can interact with.
type Target interface {
// ChainID returns the chain ID of the target contract.
ChainID() uint64

// Address returns the address of the target contract.
Address() common.Address

// IsAllowedCall returns true if the call is allowed.
IsAllowedCall(call bindings.SolveCall) bool

// TokenPrereqs returns the token prerequisites required for the call.
TokenPrereqs(call bindings.SolveCall) ([]bindings.SolveTokenPrereq, error)

// Verify returns an error if the call should not be fulfilled.
Verify(srcChainID uint64, call bindings.SolveCall, deposits []bindings.SolveDeposit) error
}

type Targets []Target

func (t Targets) ForCall(call bindings.SolveCall) (Target, error) {
var match Target
var matched bool

for _, target := range t {
if target.IsAllowedCall(call) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should error if multiple targets are matched?

Copy link
Collaborator

Choose a reason for hiding this comment

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

mmm, should we use the inbox or outbox contract address to identify the target? Picking any target that matches feels a bit loose?

Copy link
Collaborator

Choose a reason for hiding this comment

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

non matching calls will also be hard to debug since we can't see what rules are expected and why they are failing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should error if multiple targets are matched?

Yeah would help us catch bugs / bad target config.

mmm, should we use the inbox or outbox contract address to identify the target? Picking any target that matches feels a bit loose?

We will still be executing through the outbox. And there is a difference between outbox allowing or not allowing a call (which it does). And a target saying "yes this is an allowed call".

We can also add an outbox check.

non matching calls will also be hard to debug since we can't see what rules are expected and why they are failing

Yeah good point we do need to consider debugging.

"is this call one we support?" (target.IsAllowedCall)

Here we filter out chains / contracts / functions selectors we don't support. Here I think we can

Request for unsupported call   dest_chain="holeskey" target_contract="0x..." target_selector="0x..."

then "should I make this supported call?" (target.Verify)

Here we can log the error returned by verify

Request rejected for target      reason="insufficient deposits"   target="symbiotic" dest_chain="holeskey" ...

if matched {
return nil, errors.New("multiple targets found")
}

match = target
matched = true
}
}

if !matched {
return nil, errors.New("no target found")
}

return match, nil
}
Loading