Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

Simplify unexpiration logic in convertValidationResultsIntoOrderEvents #856

Merged
merged 17 commits into from
Jul 14, 2020
Merged

Simplify unexpiration logic in convertValidationResultsIntoOrderEvents #856

Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f7153a8
Name and use isOrderUnexpired
Jul 8, 2020
153461c
Use newFillableAmount
Jul 8, 2020
04fbfe0
Replace nil with newFillableAmount
Jul 8, 2020
178a798
Consolidate checks for rewatchOrder
Jul 8, 2020
8791f48
Remove checks that are always true
Jul 8, 2020
991a69d
Move database updates into unexpiration check
Jul 8, 2020
788e8db
Rename to timeStampIsValid and remove unneeded check
Jul 8, 2020
00ff1f6
typos
Jul 8, 2020
26d82c5
time stamp -> timestamp and correct comment
Jul 8, 2020
8616a0f
Clarify comment
z2trillion Jul 9, 2020
4101686
fix typo
z2trillion Jul 9, 2020
54a5bb3
Remove unused private function
Jul 9, 2020
4931b64
Remove nil check for non-nil argument
Jul 9, 2020
dd1dd2d
Rename function
Jul 10, 2020
a96329d
Re-add comments
Jul 14, 2020
4e17262
Rename to expirationTimeIsValid
Jul 14, 2020
7eeb2e2
Expand comment
z2trillion Jul 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 25 additions & 61 deletions zeroex/orderwatch/order_watcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1158,89 +1158,53 @@ func (w *Watcher) convertValidationResultsIntoOrderEvents(
oldAmountIsMoreThenNewAmount := oldFillableAmount.Cmp(newFillableAmount) == 1

if oldFillableAmount.Cmp(big.NewInt(0)) == 0 {
// A previous event caused this order to be removed from DB because it's
// A previous event caused this order to be removed from DB because its
// fillableAmount became 0, but it has now been revived (e.g., block re-org
// causes order fill txn to get reverted). We need to re-add order and emit an event.
w.rewatchOrder(order, acceptedOrderInfo.FillableTakerAssetAmount, validationBlock)
w.rewatchOrder(order, newFillableAmount, validationBlock)
orderEvent := &zeroex.OrderEvent{
Timestamp: validationBlock.Timestamp,
OrderHash: acceptedOrderInfo.OrderHash,
SignedOrder: order.SignedOrder(),
FillableTakerAssetAmount: acceptedOrderInfo.FillableTakerAssetAmount,
FillableTakerAssetAmount: newFillableAmount,
EndState: zeroex.ESOrderAdded,
ContractEvents: orderHashToEvents[order.Hash],
}
orderEvents = append(orderEvents, orderEvent)
} else {
// The order is expired if ExpirationTimeSeconds is equal to or less than the timestamp
// The time stamp is valid if ExpirationTimeSeconds is equal to or greater than the time stamp
z2trillion marked this conversation as resolved.
Show resolved Hide resolved
// of the validation block.
validationBlockTimeStampSeconds := big.NewInt(validationBlock.Timestamp.Unix())
isOrderExpired := order.ExpirationTimeSeconds.Cmp(validationBlockTimeStampSeconds) != 1
timeStampIsValid := order.ExpirationTimeSeconds.Cmp(validationBlockTimeStampSeconds) == 1
z2trillion marked this conversation as resolved.
Show resolved Hide resolved
isOrderUnexpired := order.IsRemoved && timeStampIsValid
z2trillion marked this conversation as resolved.
Show resolved Hide resolved

if oldFillableAmount.Cmp(newFillableAmount) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like in the new version we do not check oldFillableAmount.Cmp(newFillableAmount) == 0 before considering the order unexpired. Is there a reason this check is not required? Or to phrase it another way, do we know why this check was here in the old version of the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, in addition to checking for unexpiration, we check for the three cases (oldFillableAmount.Cmp(newFillableAmount) == 0, oldFillableAmount.Cmp(newFillableAmount) != 0 && oldAmountIsMoreThenNewAmount, and oldFillableAmount.Cmp(newFillableAmount) != 0 && !oldAmountIsMoreThenNewAmount) and do the same thing in all three cases. Since we've covered all the possibilities, this comparison check is not needed at all.

// If order was previously expired, check if it has become unexpired
if order.IsRemoved && oldFillableAmount.Cmp(big.NewInt(0)) != 0 && !isOrderExpired {
w.rewatchOrder(order, nil, validationBlock)
orderEvent := &zeroex.OrderEvent{
Timestamp: validationBlock.Timestamp,
OrderHash: order.Hash,
SignedOrder: order.SignedOrder(),
FillableTakerAssetAmount: order.FillableTakerAssetAmount,
EndState: zeroex.ESOrderUnexpired,
}
orderEvents = append(orderEvents, orderEvent)
}
// No important state-change happened. Still want to update lastValidatedBlock
w.updateOrderLastValidatedBlock(order, validationBlock)
continue
}
if oldFillableAmount.Cmp(big.NewInt(0)) == 1 && oldAmountIsMoreThenNewAmount {
// If order was previously expired, check if it has become unexpired
if order.IsRemoved && oldFillableAmount.Cmp(big.NewInt(0)) != 0 && !isOrderExpired {
w.rewatchOrder(order, newFillableAmount, validationBlock)
orderEvent := &zeroex.OrderEvent{
Timestamp: validationBlock.Timestamp,
OrderHash: order.Hash,
SignedOrder: order.SignedOrder(),
FillableTakerAssetAmount: order.FillableTakerAssetAmount,
EndState: zeroex.ESOrderUnexpired,
}
orderEvents = append(orderEvents, orderEvent)
} else {
w.updateOrderFillableTakerAssetAmount(order, newFillableAmount, validationBlock)
}
// Order was filled, emit event
if isOrderUnexpired {
w.rewatchOrder(order, newFillableAmount, validationBlock)
orderEvent := &zeroex.OrderEvent{
Timestamp: validationBlock.Timestamp,
OrderHash: acceptedOrderInfo.OrderHash,
OrderHash: order.Hash,
SignedOrder: order.SignedOrder(),
EndState: zeroex.ESOrderFilled,
FillableTakerAssetAmount: acceptedOrderInfo.FillableTakerAssetAmount,
ContractEvents: orderHashToEvents[order.Hash],
FillableTakerAssetAmount: order.FillableTakerAssetAmount,
albrow marked this conversation as resolved.
Show resolved Hide resolved
EndState: zeroex.ESOrderUnexpired,
}
orderEvents = append(orderEvents, orderEvent)
} else if oldFillableAmount.Cmp(big.NewInt(0)) == 1 && !oldAmountIsMoreThenNewAmount {
// The order is now fillable for more then it was before. E.g.: A fill txn reverted (block-reorg)
// If order was previously expired, check if it has become unexpired
if order.IsRemoved && oldFillableAmount.Cmp(big.NewInt(0)) != 0 && !isOrderExpired {
w.rewatchOrder(order, newFillableAmount, validationBlock)
orderEvent := &zeroex.OrderEvent{
Timestamp: validationBlock.Timestamp,
OrderHash: order.Hash,
SignedOrder: order.SignedOrder(),
FillableTakerAssetAmount: order.FillableTakerAssetAmount,
EndState: zeroex.ESOrderUnexpired,
}
orderEvents = append(orderEvents, orderEvent)
} else {
w.updateOrderFillableTakerAssetAmount(order, newFillableAmount, validationBlock)
} else {
w.updateOrderFillableTakerAssetAmount(order, newFillableAmount, validationBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

The way this is written is somewhat confusing. At this point in the code, we have not checked if the fillable amount actually changed, and yet we are still calling a function to update it in the database. It's also slightly less efficient since it is potentially updating a field in the database that doesn't actually need to change (though I'm less concerned about that part).

Can you refactor this or maybe just rename updateOrderFillableTakerAssetAmount to make this more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it to updateOrderFillableTakerAssetAmountAndBlockInfo. I'm going to make another PR that unifies updateOrderFillableTakerAssetAmountAndBlockInfo, rewatchOrder, and unwatchOrder, since they're all the same, except for how they handle IsRemoved.

}

if oldFillableAmount.Cmp(newFillableAmount) == 0 {
continue
} else {
endState := zeroex.ESOrderFillabilityIncreased
if oldAmountIsMoreThenNewAmount {
endState = zeroex.ESOrderFilled
}
orderEvent := &zeroex.OrderEvent{
Timestamp: validationBlock.Timestamp,
OrderHash: acceptedOrderInfo.OrderHash,
SignedOrder: order.SignedOrder(),
EndState: zeroex.ESOrderFillabilityIncreased,
FillableTakerAssetAmount: acceptedOrderInfo.FillableTakerAssetAmount,
EndState: endState,
FillableTakerAssetAmount: newFillableAmount,
ContractEvents: orderHashToEvents[order.Hash],
}
orderEvents = append(orderEvents, orderEvent)
Expand Down Expand Up @@ -1636,7 +1600,7 @@ func (w *Watcher) permanentlyDeleteOrder(order *types.OrderWithMetadata) error {
return err
}

// After permanently deleting an order, we also remove it's assetData from the Decoder
// After permanently deleting an order, we also remove its assetData from the Decoder
err := w.removeAssetDataAddressFromEventDecoder(order.MakerAssetData)
if err != nil {
// This should never happen since the same error would have happened when adding
Expand Down Expand Up @@ -1811,7 +1775,7 @@ func (w *Watcher) getLatestBlockFromEvents(events []*blockwatch.Event) *types.Mi
return latestBlock
}

// WaitForAtLeastOneBlockToBeProcessed waits until the OrderWatcher has processed it's
// WaitForAtLeastOneBlockToBeProcessed waits until the OrderWatcher has processed its
// first block
func (w *Watcher) WaitForAtLeastOneBlockToBeProcessed(ctx context.Context) error {
select {
Expand Down