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

Remove expiration buffer config and emit expiry events based on block timestamp #490

Merged
merged 17 commits into from Nov 5, 2019

Conversation

@fabioberger
Copy link
Contributor

fabioberger commented Nov 4, 2019

Fixed: #419 and #418

This PR:

  • Refactors Mesh to emit order expiration events when orders are expired according to the latest block timestamp. We no longer emit UTC-based expiration events.
  • An order can now become unexpired, if a block re-org causes the latest block timestamp to be earlier than it's predecessor. We therefore added the UNEXPIRED order event type.
  • Removes the expiration buffer config as it is no longer needed.
@fabioberger fabioberger force-pushed the refactor/removeBufferRenameExpiry branch from 5541352 to 783ac59 Nov 5, 2019
@fabioberger fabioberger changed the title WIP: Remove expiration buffer config and rename expiry event Remove expiration buffer config and emit expiry events based on block timestamp Nov 5, 2019
Copy link
Member

albrow left a comment

Mostly looks good to me. Just suggested a few small changes.

expirationwatch/expiration_watcher.go Outdated Show resolved Hide resolved
ChainID: config.EthereumChainID,
MaxOrders: config.MaxOrdersInStorage,
MaxExpirationTime: metadata.MaxExpirationTime,
LatestBlockTimestamp: latestBlockTimestamp,

This comment has been minimized.

Copy link
@albrow

albrow Nov 5, 2019

Member

Does LatestBlockTimestamp really need to be passed in as part of Config on startup? It seems like:

  1. Unless there was a sudden unexpected shutdown, we will have already pruned any expired orders when the block watcher notified us of this block before Mesh was shutdown.
  2. The block watcher is going to notify us of the next latest block in a matter of seconds (or less) anyways.

This comment has been minimized.

Copy link
@fabioberger

fabioberger Nov 5, 2019

Author Contributor

We could potentially miss a block re-org caused Unexpired event emission since on restart the OrderWatcher would not know what the latestBlockTimestamp was before the shutdown. It's an edge-case for sure, I just wanted to make sure the implementation was bullet proof. If you don't want to handle this edge-case, I can get rid of it.

zeroex/orderwatch/order_watcher.go Outdated Show resolved Hide resolved
zeroex/orderwatch/order_watcher_test.go Outdated Show resolved Hide resolved
fabioberger and others added 5 commits Nov 5, 2019
Co-Authored-By: Alex Browne <stephenalexbrowne@gmail.com>
…ect/0x-mesh into refactor/removeBufferRenameExpiry
…il to detect when it's not yet been set
…ion since the logic behaves correctly with it being set by the first event received
@albrow albrow self-requested a review Nov 5, 2019
@albrow
albrow approved these changes Nov 5, 2019
Copy link
Member

albrow left a comment

Approved after CI passes.

@fabioberger fabioberger merged commit fbe81c0 into development Nov 5, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@fabioberger fabioberger deleted the refactor/removeBufferRenameExpiry branch Nov 5, 2019
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.