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

[Builtins] Added 'Data' as a built-in type #3385

Merged

Conversation

effectfully
Copy link
Contributor

Don't look here yet.

@effectfully effectfully force-pushed the effectfully/builtins/add-data branch from 932ff8a to cfd2f2f Compare June 16, 2021 19:16
@effectfully effectfully force-pushed the effectfully/builtins/add-data branch 3 times, most recently from 47ec54b to e2aea26 Compare June 18, 2021 20:28
Comment on lines 95 to 98
instance Serialise a => Flat (AsSerialize a) where
encode = encode . serialise . unAsSerialize
decode = AsSerialize . deserialise <$> decode
size = error "Implement me"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelpj what about size?

Copy link
Contributor

Choose a reason for hiding this comment

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

We're encoding a lazy bytestring, so presumably size @ByteString. Or you could call https://hackage.haskell.org/package/flat-0.4.4/docs/Flat-Encoder-Strict.html#v:sLazyBytes which is how size is implemented for lazy bytestrings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This:

newtype AsSerialize a = AsSerialize
    { unAsSerialize :: a
    } deriving newtype (Serialise)

instance Serialise a => Flat (AsSerialize a) where
    encode = encode . serialise
    decode = deserialise <$> decode
    size = size . serialise

? Also pinging @kwxm and @raduom who I think also did something with flat (for context: we're trying to write a Flat instance for things that already have a Serialize instance).

@effectfully effectfully force-pushed the effectfully/builtins/add-data branch from e2aea26 to bff5501 Compare June 20, 2021 11:13
@effectfully
Copy link
Contributor Author

Ready for review. nix stuff is broken and I'm away from my nix-polluted machine, so I won't be able to fix it any time soon.

@effectfully effectfully marked this pull request as ready for review June 21, 2021 13:00
@effectfully effectfully requested review from michaelpj and kwxm June 21, 2021 13:00
Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Generally looks good.

Copy link
Contributor

@kwxm kwxm left a comment

Choose a reason for hiding this comment

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

Looks OK, and it'll be interesting to see what happens when it's integrated with the plugin. Presumably it'll be faster and smaller: would it be easy to generate some random examples to try it on right now before we do it for real? It's probably not worth the bother, but if it's something that could be done in 20 minutes if might be interesting to check.

I worry a little bit about costing, since if if we get this in the plugin and we don't have any budgeting in place it'll mess up the cost model (although now that I think about it, it looks as if builtins typically only take up 2 or 3% of the execution time, so if reading data's really quick it might be safe to ignore it for a bit). I couldn't see any issues for this, so if there's nothing relevant maybe we should make one and think about it a bit (there's an issue about how lists and Data advertise their size to the budgeting mechanism, for example). On the other hand, I should try to finalise the cost model in its current form before we start adding complicated new stuff. I hope to get that done by the end of this sprint.

plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs Outdated Show resolved Hide resolved
plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs Outdated Show resolved Hide resolved
@@ -85,6 +88,15 @@ implementations for them (if they have any constructors reserved for future use)
By default, Flat does not use any space to serialise `()`.
-}

newtype AsSerialize a = AsSerialize
{ unAsSerialize :: a
Copy link
Contributor

@kwxm kwxm Jun 25, 2021

Choose a reason for hiding this comment

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

[Comment deleted: I didn't understand what was going on here, but then spotted the previous conversation with @michaelpj.]

I wonder if doing it this way is significantly more expensive or larger than having a special instance?

Also, how are Data objects going to be transmitted? I had a vague idea that they were going to be Flat wrapped in CBOR, but the earlier conversation above suggests that there's already a full CBOR instance. What is that for? If it's for transmission, why do we need a Flat instance at all? Because we might have programs with Data objects inside them?

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've no idea what I'm doing, I only implemented @michaelpj's suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Data has a CBOR instance, because it's going to appear in a number of other places in transactions etc, and also it's literally designed to be a subset of CBOR. People may want to generate it using other tools etc.
  • We need all types in the builtin universe to have Flat instances, so Data also needs a Flat instance. As Kenneth says, indeed this is so that we could serialize a program with a Data literal in it. For example, the fully-applied scripts produced by the plutus-use-cases-scripts thing will be like that.
  • We could give Data a new Flat instance, but given that we already have a way of serializing it, we might as well just dump it in as an opaque serialized bytestring. That has the advantage that you could in principle fish it out and work with it separately, or whatever.

@michaelpj
Copy link
Contributor

Just realised we will almost certainly need equalsData too.

@effectfully
Copy link
Contributor Author

It's green! Comments addressed. Remaining stuff is parsing and pretty-printing of Data, which I hope could be done in a different PR, and I still have no idea if

newtype AsSerialize a = AsSerialize
    { unAsSerialize :: a
    } deriving newtype (Serialise)

instance Serialise a => Flat (AsSerialize a) where
    encode = encode . serialise
    decode = deserialise <$> decode
    size = size . serialise

is correct (in particular, the definition of size).

Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Honestly, the main part is surprisingly painless!

--
-- > /\(a :: *) -> \(f : a -> a) (p : pair a a) ->
-- > biconstPair {a} {a} (f (fst {a} {a} p)) (f (snd {a} {a} p)) p
obothPair :: TermLike term TyName Name DefaultUni (Either DefaultFun ExtensionFun) => term ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have uncurry too? I think that's probably the most useful function for me, since I'll want to write builtinPairToHaskellPair which is more-or-less uncurryBuiltin haskellPairConstructor

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it's in the stdlib bit. I don't really understand why this isn't there, then? Whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So stdlib is for people to use, examples is for me to write tests. Monomorphic mapping is a weird thing, so I don't want people to use it (unless they really need to, in which case it should be moved to stdlib), hence it lives in examples.

its @Data@ argument and chooses the appropriate branch (type instantiations and strictness concerns
are omitted for clarity):

chooseData
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me wonder whether we should have chooseList instead of null for consistency (noting that ifThenElse is exactly chooseBool). That would give us a consistent story for "how to do built-in datatypes": have a chooseFoo function and then an unsafeUnConstrX function for each constructor.

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 kinda like how if null xs then z else f (head xs) (tail xs) reads, but I wouldn't mind using chooseList. But let's do that separately then.

@@ -85,6 +88,15 @@ implementations for them (if they have any constructors reserved for future use)
By default, Flat does not use any space to serialise `()`.
-}

newtype AsSerialize a = AsSerialize
{ unAsSerialize :: a
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Data has a CBOR instance, because it's going to appear in a number of other places in transactions etc, and also it's literally designed to be a subset of CBOR. People may want to generate it using other tools etc.
  • We need all types in the builtin universe to have Flat instances, so Data also needs a Flat instance. As Kenneth says, indeed this is so that we could serialize a program with a Data literal in it. For example, the fully-applied scripts produced by the plutus-use-cases-scripts thing will be like that.
  • We could give Data a new Flat instance, but given that we already have a way of serializing it, we might as well just dump it in as an opaque serialized bytestring. That has the advantage that you could in principle fish it out and work with it separately, or whatever.

plutus-core/plutus-core/src/PlutusCore/Flat.hs Outdated Show resolved Hide resolved
@effectfully
Copy link
Contributor Author

Honestly, the main part is surprisingly painless!

The entire thing would've been completely painless if we didn't support polymorphic functions over polymorphic builtins, 'cause it's the part that requires all the Esc, HasUniApply, built-in types normalization, complex KnownType instances etc business.

@effectfully effectfully merged commit b2ea280 into IntersectMBO:master Jul 1, 2021
@effectfully effectfully deleted the effectfully/builtins/add-data branch July 1, 2021 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants