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 all 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
116 changes: 35 additions & 81 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,60 @@ 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 order expiration time is valid if it is greater than the latest block timestamp
// of the validation block.
validationBlockTimeStampSeconds := big.NewInt(validationBlock.Timestamp.Unix())
isOrderExpired := order.ExpirationTimeSeconds.Cmp(validationBlockTimeStampSeconds) != 1
validationBlockTimestampSeconds := big.NewInt(validationBlock.Timestamp.Unix())
expirationTimeIsValid := order.ExpirationTimeSeconds.Cmp(validationBlockTimestampSeconds) == 1
isOrderUnexpired := order.IsRemoved && expirationTimeIsValid

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
// We can tell that an order was previously expired if it was marked as removed with a non-zero fillable amount. There is no other explanation for this database state. The order is considered "unexpired" if it was previously expired but now has a valid expiration time based on the latest block timestamp.
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.updateOrderFillableTakerAssetAmountAndBlockInfo(order, newFillableAmount, validationBlock)
}

if oldFillableAmount.Cmp(newFillableAmount) == 0 {
// No important state-change happened. Note that either rewatchOrder or
// updateOrderFillableTakerAssetAmountAndBlockInfo in the unexpiration logic has already
// updated lastValidatedBlock.
continue
} else {
// Either the fillable amount has increased, e.g. a fill transaction reverted
// because of a block reorg, or it has decreased because of a partial or complete
// fill.
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 @@ -1558,22 +1529,7 @@ func validateOrderSize(order *zeroex.SignedOrder) error {

// TODO(albrow): Add tests for LastValidatedBlockNumber and LastValidatedBlockHash for
// this and other similar functions.
func (w *Watcher) updateOrderLastValidatedBlock(order *types.OrderWithMetadata, validationBlock *types.MiniHeader) {
err := w.db.UpdateOrder(order.Hash, func(orderToUpdate *types.OrderWithMetadata) (*types.OrderWithMetadata, error) {
orderToUpdate.LastUpdated = time.Now().UTC()
orderToUpdate.LastValidatedBlockNumber = validationBlock.Number
orderToUpdate.LastValidatedBlockHash = validationBlock.Hash
return orderToUpdate, nil
})
if err != nil {
logger.WithFields(logger.Fields{
"error": err.Error(),
"order": order,
}).Error("Failed to update order")
}
}

func (w *Watcher) updateOrderFillableTakerAssetAmount(order *types.OrderWithMetadata, newFillableTakerAssetAmount *big.Int, validationBlock *types.MiniHeader) {
func (w *Watcher) updateOrderFillableTakerAssetAmountAndBlockInfo(order *types.OrderWithMetadata, newFillableTakerAssetAmount *big.Int, validationBlock *types.MiniHeader) {
err := w.db.UpdateOrder(order.Hash, func(orderToUpdate *types.OrderWithMetadata) (*types.OrderWithMetadata, error) {
orderToUpdate.LastUpdated = time.Now().UTC()
orderToUpdate.FillableTakerAssetAmount = newFillableTakerAssetAmount
Expand All @@ -1595,9 +1551,7 @@ func (w *Watcher) rewatchOrder(order *types.OrderWithMetadata, newFillableTakerA
orderToUpdate.LastUpdated = time.Now().UTC()
orderToUpdate.LastValidatedBlockNumber = validationBlock.Number
orderToUpdate.LastValidatedBlockHash = validationBlock.Hash
if newFillableTakerAssetAmount != nil {
orderToUpdate.FillableTakerAssetAmount = newFillableTakerAssetAmount
}
orderToUpdate.FillableTakerAssetAmount = newFillableTakerAssetAmount
return orderToUpdate, nil
})
if err != nil {
Expand Down Expand Up @@ -1636,7 +1590,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 +1765,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