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

Interactions no longer require an execution plan #237

Merged
merged 2 commits into from
May 31, 2022

Conversation

nlordell
Copy link
Contributor

Fixes regression introduced in #195 (comment)

It turns out some of our solvers were sending single interactions without an execution plan. This used to be allowed and was disallowed in the aforementioned PR, breaking backwards compatibility.

Test Plan

CI

@nlordell nlordell requested a review from a team as a code owner May 31, 2022 09:36
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

I agree that we really should make that field required eventually. IMO it doesn't make sense that we should be responsible for bringing interactions in the correct order.

@fleupold
Copy link
Contributor

IMO it doesn't make sense that we should be responsible for bringing interactions in the correct order.

I don't think we are at the moment neither (basically we expect the order in the array to be the correct order). But agree that making it explicit is better.

@nlordell
Copy link
Contributor Author

nlordell commented May 31, 2022

@MartinquaXD: I agree that we really should make that field required eventually.
@fleupold: But agree that making it explicit is better.

Since "raw" interactions are in an array, I think it makes perfect sense for the array order to represent the order in which they are encoded. The issue arises for our internal HTTP solvers which may want to include raw interactions "inter-weaved" with AMM swaps.

This, however, is a specific problem related to the HTTP API used by our internal solvers, and I don't know if it makes sense for external solvers to have to deal with this extra "complexity" for things that it doesn't use. I think this can also be fixed by changing our HTTP API to something like:

{
  interactions: [
    { kind: "amm", /* what is in the AMM interaction today */ },
    { kind: "raw", /* what is in raw interactions today */ },
    { kind: "amm", ... },
    { kind: "raw", ... },
    { kind: "raw", ... },
    { kind: "raw", ... },
    { kind: "amm", ... },
  ],
}

This format ☝️ would also slightly more align with how settlements look like at the SC level.

@nlordell nlordell merged commit 77d267f into main May 31, 2022
@nlordell nlordell deleted the allow-raw-interactions-without-execution-plan branch May 31, 2022 12:25
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants