Add insertCoin and unionValue to DefaultFun and Plinth
#7334
Conversation
zliu41
had a problem deploying
to
github-pages
GitHub Actions
Failure
zliu41
had a problem deploying
to
github-pages
GitHub Actions
Failure
| go 89 = pure LengthOfArray | ||
| go 90 = pure ListToArray | ||
| go 91 = pure IndexArray | ||
| go 94 = pure InsertCoin |
There was a problem hiding this comment.
Why the gap between 94 and 97? Are we expecting it to be filled by some more Value-related functions?
There was a problem hiding this comment.
Yes. I will add them in the order written in the CIP.
There was a problem hiding this comment.
This looks fine. The process of adding new built-in functions and types to plutus-tx is a bit boilerplatey, so I wonder if it would be worth having a document or chekclist saying exactly what you have to do. We've got such things for plutus-core and plutus-ledger-api but there's a gap for plutus-tx.
I got a bit worried whether there was a danger of confusion with the Value type (and the other version here) in plutus-ledger-api, but I don't think we ever have to talk about built-in types there so maybe it doesn't matter.
| {-# INLINE insertCoinDenotation #-} | ||
| in makeBuiltinMeaning | ||
| insertCoinDenotation | ||
| (runCostingFunFourArguments . unimplementedCostingFun) |
There was a problem hiding this comment.
This will be the first four-argument builtin. Luckily the basic infrastructure for four-argument costing functions is already in place, but we'll probably have to extend this type and related functions to deal with insertValue. We probably don't want to do that until the final implementation of the Value type is decided though, although if that decision depends on costs we might have to add costing machinery for the different candidates.
There was a problem hiding this comment.
This will be a four argument builtin regardless of the underlying implementation.
There was a problem hiding this comment.
This will be a four argument builtin regardless of the underlying implementation.
Yes, but we'll have to add an extra constructor here and code to handle it here (and maybe elsewhere as well), and exactly what we add will depend on how the implementation behaves. It's probaby not a big deal, but we won't be able to re-use any existing code.
The non-built-in |
[Never mind: I think you fixed that when I was typing.] |
Will we not need that for old scripts? |
You are right, we do. Then we should rename it to avoid the confusion ( |
|
As titled