-
Notifications
You must be signed in to change notification settings - Fork 11
Reference inputs improvements #482
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
Conversation
Co-authored-by: Yann Hamdaoui <[email protected]>
d560b92
to
7c860fb
Compare
tests/Spec/ReferenceScripts.hs
Outdated
} | ||
validateTxSkel_ | ||
txSkelTemplate | ||
{ txSkelIns = Map.fromList [(oref, emptyTxSkelRedeemer `withReferenceInput` scriptOref), (scriptOref, emptyTxSkelRedeemer)], |
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 I don't follow what happens here; why do we have an additional input (scriptOref, emptyTxSkelRedeemer)
as compared to the old version? Was this just missing?
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 recently learned that reference script could actually be hold by regular inputs in addition to reference inputs. This was causing some weird validation errors somewhere in our test suite. Something about duplicated witness, because there was the same reference script in an input and in a reference input. Took me a while to figure that out, I had to ask for guidance on Intersect's discord.
In this example, I add as input the UTxO which contains the reference script. The only difference with the other similar example is that, during transaction generation, cooked will notice that this UTxO is already in the inputs of the transaction, and thus will not add it in the reference inputs of the generated transaction, to avoid duplication validation errors.
At first, I implemented a log event to log when cooked "discards" such a reference input because it already is a regular input, but I later removed it because TX generation is called many times during balancing so the full log was polluted with many instances of the same events. Additionally, what we usually log is things that are relevant to the users and in that case, as long as we do the right thing, the fact that the final transaction will have this input as regular input and not reference input is irrelevant for the user. So I decided not to find a more clever solution for this logging.
As a consequence, the "discarding" of this reference input is silent and thus this test does not make visible that it actually happened. Hence why it is a bit cryptic.
A solution to make it clearer would be to have the trace catch the generated transaction and return the number of reference inputs it contains, and then have the test ensure this amount is equal to 0. Let me know if you want to try and do it, otherwise I can.
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.
Thanks for the detailed explanation 👍 I'll let you decide how to act, I think the current version can be fine. Maybe adding a summary comment explaining that we are discarding the "duplicate" input could help future readers?
Co-authored-by: Yann Hamdaoui <[email protected]>
* update cabal.project and flake.lock * updating cabal file
This PR updates two aspects of reference inputs: