-
Notifications
You must be signed in to change notification settings - Fork 483
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
CIP-0138 array #6749
base: yura/cip-0138-builtin-array
Are you sure you want to change the base?
CIP-0138 array #6749
Conversation
1599c6d
to
074629d
Compare
074629d
to
e6dad3a
Compare
5f9e7e1
to
58f219c
Compare
58f219c
to
d48a07a
Compare
d48a07a
to
1f4f7cb
Compare
1f4f7cb
to
850bb4d
Compare
fe5c0a3
to
2ac9dba
Compare
850bb4d
to
19d137e
Compare
19d137e
to
a10f2fa
Compare
a10f2fa
to
c3b233f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't implement the builtin functions, right? It just defines them and the compiler recognizes them as builtins, but the evaluator doesn't yet know how to evaluate them. I assume that will be done in a separate commit/PR.
@@ -0,0 +1 @@ | |||
(program 1.1.0 (force indexArray [I 1, I 2, I 3] 2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do the elements inside the array have I
constructors, while the integer argument to indexArray
(2
) doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk, looks like a pretty-printing issue to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's definitely something wrong there. The UPLC syntax is completely wrong. It should be something like
(program 1.1.0 [[(force (builtin indexArray) (con (array integer) [1,2,3])] (con integer 2)])`
Oh, wait. Maybe it's an array of data
, in which case it should be
(program 1.1.0 [[(force (builtin indexArray) (con (array data) [I 1,I 2, I 3])] (con integer 2)])`
(maybe I haven't got the brackets exactly right).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We looked at this example with @bezirg and came to the conclusion that you're expecting "Classical" representation, while golden file uses "Readable" one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while golden file uses "Readable" one.
Why though? I think all (?) of the other UPLC golden files in the repository are in "Classical" form. It's not that important though.
c3b233f
to
514b31a
Compare
@@ -97,6 +97,12 @@ data ParamName = | |||
| FstPair'memory'arguments | |||
| HeadList'cpu'arguments | |||
| HeadList'memory'arguments | |||
| LengthArray'cpu'arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New parameters must be appended, not inserted in the middle.
And these names don't seem accurate - e.g., ListToArray
is linear, so there needs to be an intercept and a slope. If they aren't accurate, then I'm not sure what the point is adding them at all. @kwxm What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also these need to be added to V1 and V2 as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New parameters must be appended, not inserted in the middle.
👍🏼 I wrote a golden test which will fail if this condition is violated
And these names don't seem accurate - e.g., ListToArray is linear, so there needs to be an intercept and a slope. If they aren't accurate, then I'm not sure what the point is adding them at all.
The point is to make build green. Some parameters have to be added, otherwise CI tests fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a golden test which will fail if this condition is violated
How do we update the golden file when new parameters are added?
The point is to make build green. Some parameters have to be added, otherwise CI tests fail.
I'll let @kwxm check whether this is the right thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we update the golden file when new parameters are added?
cabal test plutus-ledger-api-test
fails if new parameter names were inserted in the middle, not appended.
cabal test plutus-ledger-api-test --test-options --accept
updates the golden file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The easiest thing to do would have been to update plutus-tx
and plutus-tx-plugin
and deal with plutus-ledger-api
in a separate PR after the costing gets done. I think it'd all get through CI in that case, although I might be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is to make build green. Some parameters have to be added, otherwise CI tests fail.
Is that true? If you put new things in futurePV
then I think they're not tested. Maybe I'm missing something though: it's difficult to remember how all this works without going through the whole process of adding something.
48a36dc
to
00e6771
Compare
ca42326
to
a0268bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sutff for plutus-tx
and plutus-tx-plugin
all looks OK (although I think there is a problem with the UPLC syntax in some of the golden tests: I'm not sure why that is becuase it's OK elsewhere). However I'd recommend not committing all of the changes to plutus-ledger-api
: I think it's enough to add the new builtins to futurePV
and leave everything else unchanged (but stash the updated tests somewhere in case we any them later!). It's not really meaningful to extend the cost model parameters until we know what they're going to be, so it's probably simpler to leave them unchaged for now. We also have to do expModInteger
, caseList
and caseData
as well as the array builtins, and it'd probably be best to do everything at once when all of the costing is finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there' an issue to update these tests. They got a bit broken when we made the interface more general for Conway. Maybe this is a good start!
@@ -84,6 +85,9 @@ clearBuiltinCostModel r = r | |||
, paramFindFirstSetBit = mempty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll probably have to modify all of this anyway for the next HF, since the cost model will have got bigger again for that. The new bitwise builtins and I think also ripemd_160
should certainly be in it, and maybe some of the other new ones as well.
Closes ##6731