CAP-0021 updates #1155
CAP-0021 updates #1155
Conversation
There was a problem hiding this comment.
@sisuresh Two small changes needed. Otherwise looks good.
@MonsieurNicolas Your explanation for why we can't use txTOO_EARLY makes sense, but from the POV of a client interacting with it, it is unfortunate, as the edge case obscures the common case, since in practice an error caused by those fields will almost always indicate too early. But I get it.
Co-authored-by: Leigh McCulloch <[email protected]>
Co-authored-by: Leigh McCulloch <[email protected]>
|
I added a couple more changes to this PR based on previous discussions in stellar/stellar-core#3377. |
core/cap-0040.md
Outdated
| This CAP allows the signer to use an empty payload. There are no issues with | ||
| signing an empty payload, so we see no need to special case this scenario. |
There was a problem hiding this comment.
We could be more specific as to who/what "There" is:
| This CAP allows the signer to use an empty payload. There are no issues with | |
| signing an empty payload, so we see no need to special case this scenario. | |
| This CAP allows the signer to use an empty payload. Ed25519 specifies no issues with | |
| signing an empty payload, so we see no need to special case this scenario. |
There was a problem hiding this comment.
I removed this section now that we switched to not allowing empty payloads.
| neither of the above conditions holds (`txTOO_EARLY` and `txTOO_LATE` | ||
| do not apply), but one of the `extraSigners` is unsatisfied, then the | ||
| transaction fails with `txBAD_AUTH`. | ||
| Because `PreconditionsV2` specifies multiple pre-conditions, there may be |
There was a problem hiding this comment.
I don't think we specify this, but I assume txBAD_SEQ takes precedence over any of these failure conditions. Is that correct?
There was a problem hiding this comment.
txBAD_SEQ is before txBAD_MIN_SEQ_AGE_OR_GAP and txBAD_AUTH but after the others.
core/cap-0021.md
Outdated
| preconditions are based on values that could later be valid (the `minTime` or | ||
| `minLedger` have not been reached yet), then the transaction is rejected with | ||
| `txTOO_EARLY`. If the failure is due to `minSeqAge` or `minSeqLedgerGap`, then |
There was a problem hiding this comment.
We should specify what happens if minSeqNum isn't reached yet, which I assume would still be txBAD_SEQ. On read of this it sort of implies it might be txTOO_EARLY?
core/cap-0021.md
Outdated
| preconditions are based on values that could later be valid (the `minTime` or | ||
| `minLedger` have not been reached yet), then the transaction is rejected with | ||
| `txTOO_EARLY`. If the failure is due to `minSeqAge` or `minSeqLedgerGap`, then | ||
| the transaction is rejected with the new `txBAD_MIN_SEQ_AGE_OR_GAP` error code. |
There was a problem hiding this comment.
It seems somewhat odd that the min seq errors are bundled. I can't think of an issue with this though. Would there be an advantage to specifying separate errors for age and gap? e.g.:
txBAD_MIN_SEQ_AGEtxBAD_MIN_SEQ_LEDGER_GAP
There was a problem hiding this comment.
They fail for the same simple reason, so separating them seems unnecessary to me.
sisuresh
force-pushed
the
cap21-updates-2
branch
from
c239310 to
d3361e7
Compare
There are two CAP-0021 updates in this PR-
minSeqAgedoesn't make sense. We also removed allClaimPredicatechanges because they added additional implementation work with providing any functional benefits.txBAD_MIN_SEQ_AGE_OR_GAPin response to this PR comment - Protocol 19 - CAP-0021 and CAP-0040 stellar-core#3377 (comment).