Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-0112? | Observe script type #749
CIP-0112? | Observe script type #749
Changes from 18 commits
27dec4d
d3ff6df
1b1a488
54d238b
c1482be
2851370
edff46c
056ba6a
122b68b
c7b311f
8ff7bc7
1f90c38
d5d1e72
52be316
2333ddb
7b1fe46
fa1a47c
31e90d1
c9decde
26694d6
2c5dc69
50f51d8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If we're making a change to the native script version, we should probably also get things on POSIX time rather than slot times.
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.
Whether that's another CIP or part of this one, i'd probably lean to a separate CIP, but it'd be a shame to see one without the other.
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.
Is there a reason why the purpose should include the index instead of the script hash like with minting policies and staking scripts? The spending purpose is the only one that does not provide the hash directly and I only ever use it to lookup the script's hash from the input being spent. It seems other developers do the same since there is now even this CIP which suggests changing the spending purpose to include the script's hash. I would much rather have the
Purpose
be: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 would argue it should be the other way around, the spending purpose should include the index of the UTxO being spent. The standard usage for the purpose in spending scripts is to find the input that is currently being validated against.
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 this makes sense for spending scripts but not for observation scripts. A
TxInInfo
contains a lot more information than just aTxOutRef
or aScriptHash
. It probably doesn't make sense to put all of that information into thePurpose
so an index place-holder makes sense. But for observation scripts, the only thing you can get from theTxInfoObservations
is aScriptHash
, and aScriptHash
fits no problem in thePurpose
. If the observation index serves no purpose other than to get the script hash for the current observation script, then doesn't it makes more sense to inline the script hash instead of using an index place-holder? Otherwise, the index is forcing the unnecessary extra step of looking up the script hash in the observation list. Do you personally have another use for the observation index in mind?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.
Yes, I agree.
Spending Script purpose should be
Spending Integer ScriptHash
and observe purpose should beObserve ScriptHash
.My original idea of the index in the observe purpose was for other scripts to be able to obtain the index of the observation they are looking for from the txInfoRedeemers but this requires the same amount of work as just searching the
txInfoObservations
directlyThere 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 is a weak spot in the current design of things to be sure. The thing that would be closest to the ledger would be to always provide a "pointer" to the thing that caused the run, in which case we should indeed probably change the
Spending
purpose to have an index instead. The current situation is a bit inconsistent; in some cases we resolve some of the information for you, in others not.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.
If we do decide to resolve information for ScriptPurpose, then we should make the information we resolve consistent across all the different constructors. I think
ScriptHash
is a good option in that case, since it is universally useful across the common script types (Minting, Spending, Staking).But I really like the idea of integer indices for everything because then we can move towards a future where we can better take advantage of the unique efficiency benefits offered to us by transaction determinism that other smart contract platforms (ie EVM) cannot have access to (especially if in the far future we have support for constant access indexed data structures directly in the script context). I feel like currently, we haven't even begun to scratch the surface of utilizing the advantages afforded to us via Cardano's transaction determinism.
For instance, since we know at the time of transaction construction what the majority of the script context will look like, we can theoretically bring most lookups from O(n) to O(1). Especially with TxOut Redeemers you can imagine a world where common lookup operations like
findOwnInput
,valueOf
,findStateThreadOutput
, are all O(1). Instead of looking up elements on-chain, we look them up off-chain and then we pass the index into the on-chain computation which then only has to check that the element at that index is indeed what we claim it is.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'm not sure what the equivalent of aplutus language version
is for native scripts but I am guessing they will also need a new version.Edit: See top-level review comment.