Handle reverts of trades

[anxolin]

Reverts affect to many entities. Depending when we implement this issue, we might have more affected entities, but at least:

• Trade: Needs the audit dates
• Order: Keeps track of the traded volume
• Price: Need to be undone. Index prices #25 (review)
• Balances: May be affected

[nlordell]

So trade reversions are tricky to handle. The main factor is that mappings don't have access to @derivedFrom fields so finding the exact trade that is being reverted is non-trivial. Add to that that technically multiple solutions can be processed in the same transaction and that the same order can be traded multiple times in the same solution, it all gets quite complicated.

In order to get around this, we will take advantage that all trade events are emitted immediately before a solution submission event. This means that we just need to keep track of the number of trades in a solution and we can deterministically reconstruct a trade ID given a solution and the trade index. Additionally, I noticed that prices and solution data was being computed by an eth_call when it can instead use the SolutionSubmission event. This will greatly simplify how prices are computed and allow them to be more easily reverted.

The plan for fully implementing this is:

• Add trades to "pending" solution and modify the ID on SolutionSubmission event
• Use SolutionSubmission event to get required solution data (objective value, burnt fees, submitter, etc.)
• Use SolutionSubmission event to add prices
• Keep track of number of trades in a solution and use @derivedFrom trade collection on the solution (allowing a trade to access its solution, useful for order -> trade -> solution for example)
• On trade reversions revert trades one at a time.
• First trade reversion can revert the solution (since there is no SolutionReversion event) by updating its reversion epoch
• Solution reversion should delete all prices.

[fleupold]

Thanks for coming up with a game plan ♟️

Add trades to "pending" solution and modify the ID on SolutionSubmission event

Do we know if there are any issues with renaming IDs after initial creation? I could imaging that GraphQL subscriptions might behave weirdly if we change IDs under the hood.

How about getting the solution ID from the batch object?

dex-subgraph/schema.graphql

Lines 198 to 205 in 00dccfa

	type Batch @entity {
	"Batch executed. Every batch will contain at least solution with the a set of trades that are executed in it"
	id: ID! # batchId
	# batch details
	startEpoch: BigInt!
	endEpoch: BigInt
	solution: Solution!
	solutions: [Solution!]! @derivedFrom(field: "batch")

This way, whenever we get a TradeReversion event we could:

• look up previous Solution from the Batch entity
• For all trades on that solution, revert the trade
• Also delete all prices that belong to this solution
• unset the Batch's solution field (to signal future trade reversions there is not more to do)

Generally I agree that price computation should happen onSolutionSubmission but that seems like a separate task.

[nlordell]

How about getting the solution ID from the batch object?

The issue is that the first trade does not know what solution ID to use, as the SolutionSubmission event is, AFAIK, the last event to be emitted by the SC. This means that the Batch won't have the solution set when the first trade is received. To be clear, this is an issue with creating the trade entities and not the trade reversions per se.

Do we know if there are any issues with renaming IDs after initial creation? I could imaging that GraphQL subscriptions might behave weirdly if we change IDs under the hood.

Good question. One thing to note is that all of these events get processed in a single block, not sure if this makes a difference or not for subscriptions. Maybe we can create a PendingSolution entity that gets created and removed solution and is not meant to be queried? Alternatively, a "pending" solution can always exist, and gets cloned on SolutionSubmission event. If Solution::trades is an @derivedFrom field, than the Trade::solution field can be updated to point to the submitted solution and the "pending" solution will automagically be "0-ed out" to to say.

For all trades on that solution, revert the trade

This also works. I just naively imagined that each TradeReversion event would revert each trade individually, but this is probably much easier to implement. One thing to note that this would mean that Solution::trades can't be an @derivedFrom field because of mappings limitation. One way to work around this if we do want to make trades an @derivedFrom field (which, IMO we do as it would allow a query where Order > Trade > Solution for example to get all solutions that traded a specific order) is we can keep track of a tradeCount and then compute each Trade::id from ${Solution::txHash}-${Solution::logIndex - i} for i in 1...Solution::tradeCount.

Generally I agree that price computation should happen onSolutionSubmission but that seems like a separate task.

The reason its related is that Price entities get created on Trade events, so they need to be removed on trade reversions as well, and potentially re-added (or not! If the new solution doesn't trade a token that and old solution did for example) with the new solution. In this sense, they need to be related to the batch/solution somehow so all prices for a given batch can be removed on reversion.

[fleupold]

The issue is that the first trade does not know what solution ID to use, as the SolutionSubmission event is

We do create the solution ID as the txhash+logIndex of the first trade event:

dex-subgraph/src/mappings/batch.ts

Lines 5 to 18 in 00dccfa

	export function createBatchIfNotCreated(batchId: BigInt, event: ethereum.Event): Batch {
	let id = batchId.toString()
	log.info('[createBatchIfNotCreated] Make sure Batch {} is created', [id])
	// If there's no batch created for the trade, means there's no solution either
	let batch = Batch.load(batchId.toString())
	if (batch == null) {
	let solutionId = toEventId(event)
	// Create batch
	batch = _createBatch(batchId, solutionId, event)
	}
	return batch!
	}

Do you think that would have to change?

One thing to note that this would mean that Solution::trades can't be an @derivedFrom field because of mappings limitation.

Does it have to be? At the moment it's just a list so I would expect I can query all trade Ids from the entity (no need to store a count on it).

Another thing I saw about the price estimation and why it might make sense to process in the trade processing is that it keeps track of cummulative volume per token (to give a sense how robust the price is).