IntersectMBO / plutus

The Plutus language implementation and tools
Apache License 2.0
1.57k stars 479 forks source link

Add specialized builtins for handling Value? #6585

Open effectfully opened 1 month ago

effectfully commented 1 month ago

@colll78 reports

lack of specialized builtins for values (builtinUnionValue, builtinValueOf, etc)

I'm not sure what the best way of doing this is though. Do we add Value as a built-in type plus some conversions to ByteStringMap or Data? Can Value change in future? In that case hardcoding the details of a specific implementation in the type signature (i.e. returning a ByteStringMap) of a conversion builtin is perhaps not wise, while returning Data is maybe fine.

Or do we just alias Value = Data in builtins?

Or maybe we can get decent performance without specialized builtins once we have ByteStringMap and some general builtins over it? I can only think of one way of doing that though, but it's not pretty: #6587.

rvcas commented 1 month ago

adding something named Value as a builtin type would be mildly annoying at least cause naming conflicts with Cek Value. I think ByteStringMap is the way for this, bringing a standard library concept down to UPLC does not seem like a good idea.

effectfully commented 1 month ago

adding something named Value as a builtin type would be mildly annoying at least cause naming conflicts with Cek Value.

Personally I think this name clash is insignificant, particularly given that it's CekValue, not just Value. Anyway, doesn't really matter and I'd be happy with a different name.

I think ByteStringMap is the way for this, bringing a standard library concept down to UPLC does not seem like a good idea.

The issue here is that unionValue seems to be a popular function and it's really hard to express it as a builtin without some heavy artillery (1, 2). And as a non-builtin it's quite expensive. Or do you think we can afford having unionValue to be inefficient?

bringing a standard library concept down to UPLC does not seem like a good idea

Perhaps. @colll78 you have a different view, don't you?

colll78 commented 1 month ago

bringing a standard library concept down to UPLC does not seem like a good idea.

This does not make sense to me. Why do you consider Value as BuiltinData to be a “standard library concept”? Maybe you have misunderstood. My proposal is to add builtin functions for working with AsBuiltinData Value.

UPLC is not a general-purpose programming language. It is a language specifically designed to write validation logic to set constraints for the creation and transfer of Value and Data across the Cardano blockchain. Given that half of the entire purpose of this language is to add constraints to the creation and transfer of Value, why should Value be a “standard library concept”?

The whole value proposition behind domain specific languages (ie UPLC) is that the design is informed by and tailored to, the problem space. As such, it should be better suited to the task in that domain than a general-purpose language. To me it seems like this has not been taken into consideration much historically in the design of Plutus. How is there not first-class language support for Values in a domain specific language designed primarily to manage the transfer of Value???

colll78 commented 1 month ago

Also BuiltinByteStringMap is not sufficient for many production use-cases, because you will have to first coerce the relevant data in the script context from BuiltinData to BuiltinList (CS (BuiltinList (TN, Amount))) and then convert it to BuiltinByteStringMap and then operate on it and then convert the inner map to BuiltinByteStringMap. Even if this wasn't the case, and they were provided directly as nested BuiltinByteStringMaps in the ScriptContext that would still not be sufficient. Values are not Maps, common Value operations function completely differently to their counterpart in a Map (ie. equality, union, lookup, fold, map) because of the nested structure and because of the invariants that Values have that Maps don't.

mpetruska commented 1 month ago

IMHO It makes sense to a dedicated Value builtin type, as it carries some additional constraints on top of just the type of the keys (e.g. the Int values are always non-negative). Also it seems that plenty of validator scripts are frequently looping over them just for checking the existence of an nft, which can be expensive in some (although maybe rare) cases. Another pro for it could be security. If some validator authors forget to cover the "token dust attack" (for example), the builtin operations might be implemented in a way to add automatic checks for it.

mpetruska commented 1 month ago

With regards to the naming/"standard library concept" issue:

keyan-m commented 1 month ago

This is a nice initiation, and I think @colll78 articulated it quite nicely. There are so many utility functions I end up writing for almost all non-trivial contracts (the one I can think of right away, is a function for validating authenticity of a UTxO, i.e. it carries a specific NFT). While I understand adding features to a language should be done conservatively, this kind of repetition is bloating the chain.

rvcas commented 1 month ago

I still just think more functions for working with PlutusData is more appropriate. The concept of Value (Multiasset thingy) doesn't exist in the VM, it's just PlutusData. But I don't feel too strongly, I just think focusing on more performant primitives is more worth it than embedding high level constructs into the machine.

The notion that Plutus VM is general purpose is silly. I was certainly not advocating for some vague notion of general purpose.

That said, the machine imo must stay decoupled from ledger implementation details. Value being one of them.

This doesn't mean we shouldn't add a builtin for this but it also doesn't need to make mention of Value in the builtin name.

I also really like the idea of higher order builtins. Sounds like a good long term play.

colll78 commented 4 weeks ago

I still just think more functions for working with PlutusData is more appropriate. The concept of Value (Multiasset thingy) doesn't exist in the VM, it's just PlutusData. But I don't feel too strongly, I just think focusing on more performant primitives is more worth it than embedding high level constructs into the machine.

I agree. I am suggesting exactly that, functions for dealing BuiltinData that is a map of inner maps where the keys are integer quantities (ie BuiltinData that corresponds to Value) that maintains the invariants of Value across these operations, namely:

The true necessary invariants afaik are:

  1. No empty maps
  2. No zero quantity inner maps
  3. Outer map is ordered lexographically by currency symbol
  4. Inner map is ordered lexographically by token name
  5. No duplicate on the token names and currency symbols

I think many Value operations can be covered by the builtins functions provided for BuiltinByteStringMap but the ones that are more value specific (ie. they work differently to the equivalent function for Maps to handle the invariants of Value). Namely, builtinUnionValue and builtinValueLessThan. I agree that limited higher order builtin functions should also be a priority.

That said, the machine imo must stay decoupled from ledger implementation details. Value being one of them.

Yep agreed. This is decoupled from the ledger Value types. If the structure of Value in the ledger changes, and toPlutusType_Value does not produce the same builtindata anymore, these builtin functions will still be useful because there will always still be PlutusV3-X scripts that use them. I also think it will be a nightmare for ecosystem tooling if a new Value is introduced to the ledger that cannot be converted to the current map of maps form (and any UTxOs with those new values will not be able to interact with PlutusV1-X contracts).