-
Notifications
You must be signed in to change notification settings - Fork 18
solana: overhaul local fills #122
Conversation
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.
Quick review, will wait for the detailed description to really dig in
processor::add_auction_history_entry(ctx) | ||
} | ||
|
||
pub fn reserve_fast_fill_sequence_active_auction( |
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.
If you get a chance to document these entrypoints, that would be awesome. It's helpful to know where they fit into the flow, expected access control, and so on
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.
Added docstrings here: 62b670b (and did a little clean up along the way). If I missed anything, let me know pls.
solana/programs/matching-engine/src/state/prepared_order_response.rs
Outdated
Show resolved
Hide resolved
}); | ||
|
||
// Now uptick sequencer's value. If this errors out, we have problems. | ||
*next_sequence = next_sequence |
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.
Is there a chance that using sequence in this way will cause similar problems that were addressed in PR #90 ? Basically if there are many orders in a short amount of time is there a possibility for confusion with the strict incremental sequencing?
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.
I wrote this before I saw the PR description above.
Maybe that flow and the race conditions involved could be adapted and added to src/lib.rs
? If not it should end up in some documentation other than GitHub eventually.
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.
Unless I'm missing something, this should not be the same issue because there is no sequence required for any of the account seeds in these instructions.
Should be good to go. Also finally added the close redeemed fast fill instruction here: 507cb40, which the PR description mentioned. |
507cb40
to
17fbfbd
Compare
b2be4fc
to
4553086
Compare
solana/programs/matching-engine/src/processor/auction/execute_fast_order/local.rs
Show resolved
Hide resolved
solana/programs/matching-engine/src/processor/auction/execute_fast_order/mod.rs
Outdated
Show resolved
Hide resolved
solana/programs/matching-engine/src/processor/auction/settle/none/local.rs
Show resolved
Hide resolved
* NOTE: This also required a modification to `AuctionSettled`
This change was very involved. Included in removing the wormhole publishing for fast fills is a way to sequence fast fill messages. Calling a new instructions called "reserve fast fill sequence" is a required step for solvers before they execute local orders.
e304bb2
to
60cabf9
Compare
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.
Business logic looks good to me after reviewing a few iterations and after internal discussions.
I did not do a detailed review of the integration test changes. The rewrite is very extensive and I think not really feasible to review in PR format. I'm trusting that the test coverage is largely similar. I'll take a look at this going forward and try to identify any gaps we have either in the happy path or in error checking.
This change is a significant departure from how local (fast) fills were handled before.
Previous Implementation
In the previous implementation, a Wormhole message with a specific fast fill payload was posted. An integrator would listen to the VAA on the guardian spy network so that he can post it in order to redeem the fill. To prevent replay attacks, a redeemed fast fill account was created. Posting the VAA + creating this replay protection account is a large cost burden to the integrator. The cost to the solver is no different than having to deliver a CCTP transfer to another chain (because that process requires posting a Wormhole message, as well).
What's Different
Instead of a Wormhole message, the matching engine emits an event via Anchor's event CPI technique (which just invokes a privileged instruction, where the program's event authority is a signer). Integrators would listen to this event so that it knows when a fast fill was delivered. This change removes the requirement for the integrator to post a VAA. And the solver has a reduced cost to deliver this fast fill since he does not have to post a Wormhole message, which creates an account.
And instead of the integrator creating a replay protection account, the matching engine creates a fast fill account with a
redeemed
boolean. When an integrator redeems the fill, this flag gets set to true. The integrator no longer spends lamports to replay protect the fast fill. The solver pays lamports to create this account.The solver can also get his lamports back by closing a fast fill account only if it is redeemed. Because the auction accounts already act as replay protection against starting new auctions, this operation should be safe.
Side Effects
In order to sequence these fast fills, an additional instruction was added to reserve a fast fill sequence number. Ideally, we would like to seed the fast fill as soon as the order is executed. But because there can be multiple fills delivered at the same time, seeding the fast fill with a sequence number can cause solver's execution to fail.
For these local fast fills, the solver needs to call the
reserve_fast_fill_sequence_active_auction
instruction prior to callingexecute_fast_order_local
instruction. He can try to be bold and call them in the same transaction. But if there are other fast fills that need to be sequenced from the same source chain and order sender, his bundled transaction will fail. The sequence reservation instruction should be called in a separate transaction prior to executing the order.Associated with this change is an addition to the Typescript SDK to listen to these particular fills. Because there is no way to listen to these event CPI instructions like with normal program log Anchor events, we added a convenient way to listen to these fast fills.
Closes #48.