Solvers solution pre/post interactions #2679
Conversation
| @@ -221,7 +227,11 @@ pub struct Solution { | |||
| #[serde_as(as = "HashMap<_, serialize::U256>")] | |||
| prices: HashMap<eth::H160, eth::U256>, | |||
| trades: Vec<Trade>, | |||
| #[serde(default)] | |||
| pre_interactions: Vec<Interaction>, | |||
There was a problem hiding this comment.
I think this is the wrong type. It doesn't make sense for solvers to specify Liquidity interactions as pre interactions. It also doesn't make sense for those to be internalizable nor specify input/output amounts. I think they should simply be
struct {
target: eth::H160,
value: eth::U256,
call_data: Vec<u8>,
}
There was a problem hiding this comment.
Switched to InteractionData
# Conflicts: # crates/driver/src/domain/competition/solution/encoding.rs
crates/solvers-dto/src/solution.rs
Outdated
| @@ -94,6 +98,14 @@ pub enum Interaction { | |||
| Custom(CustomInteraction), | |||
| } | |||
|
|
|||
| #[derive(Debug, Serialize)] | |||
| #[serde(tag = "kind", rename_all = "camelCase")] | |||
| pub struct InteractionData { | |||
There was a problem hiding this comment.
nit: maybe
| pub struct InteractionData { | |
| pub struct Call { |
to avoid confusion with existing Interaction struct?
It would be nice to reuse this type on the CustomInteraction struct, but it seems like that would be a breaking change 😢
| @@ -357,6 +361,15 @@ pub struct CustomInteraction { | |||
| pub allowances: Vec<Allowance>, | |||
| } | |||
|
|
|||
| /// An arbitrary ethereum interaction that is required for the settlement | |||
| /// execution. | |||
| #[derive(Debug)] | |||
There was a problem hiding this comment.
Didn't get it. To reuse something from the driver crate in what sense?
There was a problem hiding this comment.
You are redifining a struct in the domain which already exists in the domain. Now, we might not want to overload structs that happen to have the same fields (generally defining new structs for different semantics is good), but here this is similar to re-defining TokenAmount (which arguably is never going to change).
| @@ -357,6 +361,15 @@ pub struct CustomInteraction { | |||
| pub allowances: Vec<Allowance>, | |||
| } | |||
|
|
|||
| /// An arbitrary ethereum interaction that is required for the settlement | |||
| /// execution. | |||
| #[derive(Debug)] | |||
There was a problem hiding this comment.
You are redifining a struct in the domain which already exists in the domain. Now, we might not want to overload structs that happen to have the same fields (generally defining new structs for different semantics is good), but here this is similar to re-defining TokenAmount (which arguably is never going to change).
| pub interactions: Vec<Interaction>, | ||
| pub post_interactions: Vec<InteractionData>, |
There was a problem hiding this comment.
It would lead to this:
| pub post_interactions: Vec<InteractionData>, | |
| pub post_interactions: Vec<eth::Interaction>, |
There was a problem hiding this comment.
But eth::Interaction is part of the driver crate, and the solver crate should not depend on the driver, right? Do you want me to extract this type into a shared crate, like model?
There was a problem hiding this comment.
Oh, you are completely right (I missed that this was a different create). We could still add this type in the eth module of the solvers crate but not super important.
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
# Conflicts: # crates/driver/src/infra/solver/dto/solution.rs
squadgazzz
force-pushed
the
solution-pre-post-interactions
branch
from
8abde2c to
3059327
Compare
Description
Adds the ability for solvers to provide pre and post-interactions for a solution.
How to test
TBD
Related Issues
Fixes #2636
Further implementation
gnosis/solvers