-
Notifications
You must be signed in to change notification settings - Fork 375
CIP-0167? | Remove isValid from transactions
#1089
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-0167? | Remove isValid from transactions
#1089
Conversation
e70cf11 to
d655572
Compare
isValid flag
isValid flagisValid flag
isValid flagisValid flag from standalone transaction serialization
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 again @teodanciu @lehins - tagging Triage for introduction at next meeting (https://hackmd.io/@cip-editors/120).
@lehins @WhatisRT I think we're now seeing a greater bandwidth of Ledger CIPs for atomic changes as I think was requested in some statements I attempted to summarise in Ledger participation and bandwidth.
- Assuming we stay with the usual (and originally recommended) process of CIPs for all Ledger changes, is there a standard list of people we can tag to confirm these PRs before what will usually be, if no objection, a summary process for candidacy & merge?
- @Crypto2099 @Ryun1 @perturbing we should also collect names into this list with other node Ledger implementors to be sure it's evenly distributed across all node projects.
@teodanciu I've suggested a rephrase of the CIP title to:
- use the briefest possible phrasing of the change, as appearing at the beginning of the Rationale;
- since this CIP represents a change, like
UpdateCIPs it's more clear to express it as an action (i.e. a verb phrase: whereas proposed standards for new objects & features are generally noun phrases).
I think the list of people should be decided on CIP by CIP basis, because not everyone is affected equally. For example, this CIP could use input from the Consensus team, while other ledger related CIP might need input from the Plutus team instead. |
lehins
left a comment
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.
Love it. Hopefully community will see it as a nice simplification.
Apologies I don't have a complete list: I'll do my best to tag @bwbush @dnadales @nhenin @nfrisby @jasagredo @amesgen @fraser-iohk @geo2a @abailly |
Co-authored-by: Alexey Kuleshevich <[email protected]>
Co-authored-by: Alexey Kuleshevich <[email protected]>
isValid flag from standalone transaction serializationisValid from transactions
|
The reasoning of this CIP seems sensible and it doesn't affect any specifications that we currently have, so I thumbs up from me. |
|
@teodanciu @WhatisRT we had a shift in meeting focus in favour of Leios CIP review, and also due to not having a quorum of editors present we'll have to try confirming this as |
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
isValid from transactionsisValid from transactions
rphair
left a comment
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.
@teodanciu the CIP meeting today was an additional confirmation that this proposal is generally well-liked all around. I believe it should proceed quickly enough toward merge but the normal process is to confirm this as a candidate before promotion to Last Check which I think will happen quickly enough (so approving as soon as CIP number is applied; I personally wouldn't be able to find any fault with this & see no expert objection).
Please also update the directory name to CIP-0167 and update your OP's "rendered latest proposal version" link accordingly 🎉
rphair
left a comment
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.
As per #1089 (review).
|
@teodanciu somehow an incidental discussion got this off track (my mistake I think). I don't see any obstacle to what would be a relatively simple, universally agreed-upon change: so am marking |
|
This must be done very carefully because it has the potential to break a lot of bridges and light client protocols. A mithril bridge proves inclusion of a transaction in a block, if the transaction body as present in the block does not include the validity flag, then there is no way for such a bridge to determine whether the transaction succeeded or if the collateral was taken. |
|
@colll78 the editors will definitely ensure #1089 (comment) is addressed before merging this & take any response from @lehins @teodanciu into account about how we might define "carefully" in the CIP itself. |
|
@colll78 Does my explanation above clarifies your concern? Please let me know if it doesn't and I'll be happy to discuss it more. We have this statement in the CIP, please let me know how we can make it clearer:
|
Okay so it does not impact how bridges validate whether a transaction in a block succeeded or failed? If so then yes it addresses this concern. |
Yes, that is correct. This CIP only affects mempool submission, not block validation |
|
@teodanciu @lehins at |
Ryun1
left a comment
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.
approving
assuming that directory rename will be done by @rphair 🙌
Omit
isValidflag from the standalone transaction encoding (not affecting the block serialization)(rendered latest proposal version)