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

Time changes: no more builtin ms and tweaks. #233

Merged
merged 30 commits into from
Feb 28, 2023
Merged

Conversation

mmontin
Copy link
Collaborator

@mmontin mmontin commented Jan 30, 2023

This PR is both:

@mmontin mmontin changed the base branch from ch/hedge-v2 to main February 2, 2023 14:37
@mmontin mmontin changed the title Parameters in BlockChain + Ms no longer builtin New time management Feb 2, 2023
@mmontin mmontin changed the title New time management Time changes: no more builtin ms and tweaks. Feb 2, 2023
@carlhammann carlhammann mentioned this pull request Feb 23, 2023
@mmontin mmontin marked this pull request as ready for review February 23, 2023 14:43
Copy link
Contributor

@carlhammann carlhammann left a comment

Choose a reason for hiding this comment

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

This PR is a nice simplification of the API of MonadBlockChain, and also contains a few useful tweaks. As discussed, I left my review in form of a few commits 😉

Copy link
Member

@facundominguez facundominguez left a comment

Choose a reason for hiding this comment

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

I made some comments on the validity range of auctions that, if addressed, could expose the bug we observed in cooked v1.

cooked-validators/src/Cooked/MockChain/BlockChain.hs Outdated Show resolved Hide resolved
cooked-validators/src/Cooked/MockChain/BlockChain.hs Outdated Show resolved Hide resolved
examples/src/Auction/Offchain.hs Outdated Show resolved Hide resolved
examples/src/Auction/Offchain.hs Show resolved Hide resolved
examples/tests/AuctionSpec.hs Outdated Show resolved Hide resolved
@Niols Niols self-requested a review February 24, 2023 16:51
Comment on lines 323 to 329
-- | Return the slot that contains the given time. It holds that
--
-- > slotToTimeInterval (getEnclosingSlot t) == (a, b) ==> a <= t <= b
--
-- and
--
-- > slotToTimeInterval n == (a, b) ==> getEnclosingSlot a == n && getEnclosingSlot b == n
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- | Return the slot that contains the given time. It holds that
--
-- > slotToTimeInterval (getEnclosingSlot t) == (a, b) ==> a <= t <= b
--
-- and
--
-- > slotToTimeInterval n == (a, b) ==> getEnclosingSlot a == n && getEnclosingSlot b == n
-- | Return the slot that contains the given time.
--
-- See 'slotToTimeInterval' for its relationship to 'getEnclosingSlot'.

Perhaps we could avoid maintaining two copies of these properties.

Ledger.Slot nr <- getEnclosingSlot r
return (nl, nr) of
Left _err -> False
Right ((nl, nr), _) -> nl == 42 && nr == 42,
Copy link
Member

Choose a reason for hiding this comment

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

Why 42 instead of some arbitrary integer? (Similar to the property-based test below)

@@ -86,7 +86,7 @@ txBid submitter offerOref bid = do
Just deadline = A.getBidDeadline datum
seller = A.getSeller datum
lotPlusPreviousBidPlusNft = outputValue output
validityInterval <- slotRangeBefore deadline
validityInterval <- slotRangeBefore (deadline - 1)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this remain slotRangeBefore deadline? The bid validator should accept a transaction on the deadline if the node interprets slots the same as the off-chain code.

@@ -149,7 +149,7 @@ txHammer submitter offerOref = do
let datum = output ^. outputDatumL
Just deadline = A.getBidDeadline datum
seller = A.getSeller datum
validityInterval <- slotRangeAfter deadline
validityInterval <- slotRangeAfter (deadline + 1)
Copy link
Member

Choose a reason for hiding this comment

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

Similar question here.

@facundominguez
Copy link
Member

The change in the meaning of slotRangeAfter and slotRangeBefore is looking good to me. I'm getting picky with the validity ranges in the Auction example now.

cooked-validators/tests/Cooked/MockChain/BlockChainSpec.hs Outdated Show resolved Hide resolved
examples/tests/AuctionSpec.hs Outdated Show resolved Hide resolved
@carlhammann carlhammann merged commit 29b7646 into main Feb 28, 2023
@carlhammann carlhammann deleted the mm/ms-to-slots branch February 28, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

POSIXTime vs Slot
5 participants