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

orderwatch,rpc: Supply parsed contract events in emitted OrderEvents #420

Merged
merged 17 commits into from Oct 3, 2019

Conversation

@fabioberger
Copy link
Contributor

fabioberger commented Sep 17, 2019

Fixes: #396

This PR adds the parsed order-relevant contract events to the OrderEvents emitted from the JSON-RPC orders subscription. At first we thought we might be able to emit a top-level order event for every contract event. This is however not possible to do in a robust way. The reason for this is two fold:

  1. We are only able to execute eth_call requests at the boundaries between blocks. We are unable to execute them at particular transaction indexes within blocks. What this means is that we can only query the state of orders before or after all transactions (and their associated events) within a block have been processed. This leaves us with an end state for each order after the block is processed, and we are tasked with figuring out how the individual contract events emitted contributed to this total state update.

  2. In order to figure out how each event impacts the orders state, we could fetch the maker's balances/allowances at currentBlockNumber - 1, and then implement a simulator that applies every inferred state change of each contract events to these values, as well as the fillableTakerAssetAmount retrieved from the DB. From these values, we could then compute the state of the order. The glaring issue with this approach is that sady contract events are not guarenteed to line up perfectly with the actual state changes taking place on-chain (e.g., if an order involves transferring a token that doesn't adhere perfectly to say the ERC20 standard, we won't be notified of it's balance/allowance changes). This means we could hit logical inconsistencies between our simulated state of the chain and it's actual state, leading to inaccuracies in how we map the contract events back to order state changes.

Thus, I've decided to expose all the decoded order-relevant contract events together with the top-level OrderEvent which represents the overall state change of the order. We do not try to elucidate the impact of each contract event on the order (since this cannot be done reliably with the event watching approach), but we still allow the subscriber to see all discrete events related to their orders so that they can keep track of every fill/cancel as needed for the MMer use-case.

One quirk to this approach is that we might return contract events that could have but ultimately did not impact the order's state (e.g., a maker balance increase where the maker already had a sufficient balance). There is currently no reliable way to filter these out.

@fabioberger fabioberger force-pushed the refactor/orderEvents branch from 48d8593 to 11cb9a2 Sep 17, 2019
@fabioberger fabioberger marked this pull request as ready for review Sep 18, 2019
@fabioberger fabioberger changed the base branch from orderWatcherTests to development Sep 18, 2019
@fabioberger fabioberger changed the title Refactor/order events orderwatch,rpc: Supply parsed contract events in emitted OrderEvents Sep 18, 2019
@fabioberger fabioberger requested a review from albrow Sep 18, 2019
@fabioberger fabioberger self-assigned this Sep 18, 2019
@albrow

This comment has been minimized.

Copy link
Member

albrow commented Sep 19, 2019

@fabioberger While you raise some good points, I want to see if we can find a compromise that could improve QoL for market makers.

This whole thing was brought to our attention because of a specific issue. If there is a partial fill followed by a cancel in the same block, the current version of Mesh will emit a CANCELLED event but not a FILLED event. This is really important for market makers because if an order was partially filled it can impact their trading strategy. (There might be other similar situations along these lines, but this is the one I understand best).

This PR asks market makers to do the work of iterating through the contract events associated with a CANCELLED OrderEvent to figure out the order was actually partially filled before it was canceled. Given the fact that we have an OrderEvent kind called FILLED, this feels wrong. At the very least it's hard to explain and could be a common source of issues down the road. While it's not a ton of work, it does also put more onus on market makers and other users of Mesh.

I understand that some token contracts do not emit standardized events (some may not even emit events at all). And I agree that it is infeasible to rely on contract events to understand everything that could happen to an order. However, in this specific case, the related events (Fill and Cancel) are emitted by our own smart contracts so we can rely on them. Would it be feasible for Mesh to emit a FILLED OrderEvent in addition to CANCELLED in cases like this? We can still keep nearly everything else in this PR and include contract events as part of emitted OrderEvents. I'm just asking if we can emit more than one OrderEvent in certain cases based on the contract events we see?

@fabioberger

This comment has been minimized.

Copy link
Contributor Author

fabioberger commented Sep 21, 2019

@albrow what makes your ask impossible is the following scenario:

Within a single block, there are the following state transitions impacting an order:

  • MakerAsset transfer out of MakerAddress
  • Fill/Cancel of order

If there is a transfer before the fill/cancel, we cannot know with 100% certainty what to set the fillableTakerAssetAmount for the separate FILLED event. If partially fillable orders were not possible on the protocol level, this would not be an issue but unfortunately they are still possible. Similarly, we don't know what the fillableTakerAssetAmount should be for the transfer event in isolation. If we don't supply the fillableTakerAssetAmount with events, we could split them up trivially. I'd argue though that fillableTakerAssetAmount is a very valuable field and this was confirmed by the MMers I've spoken with.

@albrow

This comment has been minimized.

Copy link
Member

albrow commented Sep 23, 2019

@fabioberger how about just adding multiple event kinds to indicate that something else happened before a cancel? Something like:

{
    "jsonrpc": "2.0",
    "method": "mesh_subscription",
    "params": {
        "subscription": "0xcd0c3e8af590364c09d0fa6a1210faf5",
        "result": [
            {
                "orderHash": "0x96e6eb6174dbf0458686bdae44c9a330d9a9eb563962512a7be545c4ecc13fd4",
                "signedOrder": {
                    // ...
                },
+                "kind": ["FILLED", "CANCELLED"],
                "fillableTakerAssetAmount": 470000000000,
                "contractEvents": [
                    // ...
                ]
            }
        ]
    }
}

If we go this route, we could could probably also rename the kind field to something else.

fabioberger added 8 commits Sep 15, 2019
@albrow albrow force-pushed the refactor/orderEvents branch from cac7978 to 05aa3d2 Sep 26, 2019
@albrow

This comment has been minimized.

Copy link
Member

albrow commented Sep 26, 2019

Heads up @fabioberger this PR was pretty out of date so I rebased it on development.

…ents the end state of the order after it's revalidation since the last time it was re-validated
CHANGELOG.md Outdated Show resolved Hide resolved
browser/ts/index.ts Outdated Show resolved Hide resolved
browser/ts/index.ts Outdated Show resolved Hide resolved
docs/rpc_api.md Outdated Show resolved Hide resolved
rpc/clients/typescript/src/types.ts Show resolved Hide resolved
rpc/clients/typescript/src/types.ts Show resolved Hide resolved
zeroex/ordervalidator/order_validator.go Show resolved Hide resolved
zeroex/order.go Outdated Show resolved Hide resolved
}
switch c.Kind {
case "ERC20TransferEvent":
m["parameters"] = c.Parameters.(decoder.ERC20TransferEvent).JSValue()

This comment has been minimized.

Copy link
@albrow

albrow Oct 2, 2019

Member

This also really makes me lean toward named interface approach instead of using interface{}. If we define a ContractEventParameters interface that inherits from js.Wrapper we can remove this switch statement and all the type casts, drastically simplifying this code. It would just become:

	m := map[string]interface{}{
		"blockHash":  c.BlockHash.Hex(),
		"txHash":     c.TxHash.Hex(),
		"txIndex":    c.TxIndex,
		"logIndex":   c.LogIndex,
		"isRemoved":  c.IsRemoved,
		"kind":       c.Kind,
+		"parameters": c.Parameters.JSValue(), 
	}

The only other thing we might need to do is check if c.Parameters is nil.

zeroex/order.go Outdated Show resolved Hide resolved
zeroex/order_js.go Outdated Show resolved Hide resolved
@albrow
albrow approved these changes Oct 3, 2019
@fabioberger fabioberger merged commit d3f6cd4 into development Oct 3, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.