Skip to content

Conversation

uniphil
Copy link

@uniphil uniphil commented Sep 9, 2025

the sequence of ops used by log_nullification_nontrivial.json,

(op0)<-(op1)<-(op2)<-(op3)<-(op4)  (op5)<-(op6)  (op7)
            \             \       /             /
             \             '--<--'             /
              '-------<------------------<----'

has all ops happening within 72h, but it seems clear that

  • op0 can be arbitrarily earlier, and
  • op1 can maybe also be arbirarily earlier? (at least that's what i'd expect)

this change moves op0 and op1 outside the 72h window, so that only clobbered events are inside it

the wording in the spec about the conditions for clobbering ops was slightly unclear to me, so i tried to dig into test cases for certainty before suggesting a wording change, and ended up here. accepting or resolving this change (specifically re op1) would clarify my question about the spec.


note: i expected the test to fail when i moved op2 outside the 72h window as well, but it did not. from what i can see, the tests only asserts that a sequence of events is made up of a valid submitted ops, but not that any are accepted/rejected or that any particular state results at the end. if that's not meant to be the case, i'd love to add assertions about that!

the sequence of ops used by `log_nullification_nontrivial.json`,

```
(op0)<-(op1)<-(op2)<-(op3)<-(op4)  (op5)<-(op6)  (op7)
            \             \       /             /
             \             '--<--'             /
              '-------<------------------<----'
```

has *all* ops happening within 72h, but it seems clear that

- op0 can be arbitrarily earlier, and
- op1 can maybe also be arbirarily earlier? (at least that's what i'd expect)

this change moves op0 and op1 outside the 72h window, so that only *clobbered* events are inside it

the [wording in the spec](https://github.com/did-method-plc/did-method-plc?tab=readme-ov-file#key-rotation--account-recovery) about the conditions for clobbering ops was slightly unclear to me, so i tried to dig into test cases for certainty before suggesting a wording change, and ended up here. accepting or resolving this change (specifically re op1) would clarify my question about the spec.

---

note: i *expected* the test to fail when i moved `op2` outside the 72h window as well, but it did not. from what i can see, the tests only asserts that a sequence of events is made up of a valid sequence of submitted ops, but not that any are accepted/rejected or that any particular state results at the end. if that's not meant to be the case, i'd love to add assertions about that!
@uniphil uniphil changed the title nontriv nulli test: put non-colobbered ops >72h nontriv nulli test: put non-clobbered ops >72h Sep 9, 2025
@DavidBuchanan314
Copy link
Collaborator

DavidBuchanan314 commented Sep 9, 2025

The log_nullification_at_exactly_72h.json test case also captures this detail already, although less directly. The final op is >72h after the op referenced in prev. The 72h limit applies relative to the op being nullified (IIUC!)

I agree that the wording in the spec is unclear, in fact I think it's wrong when it says "it is submitted within 72hrs of the referenced operation".

In practice the reference implementation at https://github.com/did-method-plc/did-method-plc/tree/main/packages is more authoritative than the spec, so I will triple-check tomorrow that the behaviour there matches the test cases here - we want to update the spec to align with the implementation, but obviously we need to make sure any clarifications are correct.

I've opened an issue in the main PLC repo to track this: did-method-plc/did-method-plc#116

Edit: oops, missed that you already PR'd against the did-method-plc repo, my issue is a little redundant, oh well.

@uniphil
Copy link
Author

uniphil commented Sep 9, 2025

ok perfect, happy it's already covered there and thank you for the detailed response! it all aligns with my expectations. thank youuuuuu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants