Remove legacy use of GeneralizedTime
Opened this issue · 8 comments
The "annoyingly" encoded notBefore and notAfter times in attestations using GeneralizedTime is no longer needed, since we now use notBeforeInt and notAfterInt to store times in a away that is easy for smart contracts to use.
Furthermore since we now only issue attestations valid for an hour, there is no need to keep backward compatibility.
(Although we once issued unlimited attestations, but I think it is fair to deprecated these, if any still exist.)
This issues should only be fixed once we get a proper CI/CD setup between attestation.id, authenticator.js, attestation.jar and the smart contracts since it is not a high priority issue.
See PR #239 and this discussion .
Furthermore since we now only issue attestations valid for an hour, there is no need to keep backward compatibility.
When did this change came to be? This is too short for 2 reasons:\
-
It's not unusual that a blockchain transaction takes 1 hour or 1 day to be inserted into the blockchain. Sometimes the user can't push a new transaction until the old one is included in the blockchain, even if he knows the old one contains an already-expired attestation!
-
identifier attestations might have re-use value. Please state the scope of this issue, do you mean for all types of attestation or a few types.
Regarding point 1. the thought was that if we could make the design change relatively easy, perhaps without needing support for the old and new ASN format at the same time.
Regarding point 2. the thought was to apply this to all attestations, since the format is already modified so much that it is not x509 compliant. Hence we thought it would not be necessary to add redundant GeneralizedTime data instead of a more blockchain friendly timestamp.
- It's not unusual that a blockchain transaction takes 1 hour or 1 day to be inserted into the blockchain. Sometimes the user can't push a new transaction until the old one is included in the blockchain, even if he knows the old one contains an already-expired attestation!
Current Issue related to remove unused fields notBefore and notAfter , not related to the attestation TTL
anyway - we use attestation TTL 1 hour for test purposes, last Auth0 attestation TTL is 30 days.
- identifier attestations might have re-use value. Please state the scope of this issue, do you mean for all types of attestation or a few types.
currently identifier attestations reusable, its OK
@weiwu-zhang , @jot2re , what do we decide? remove redundant notBefore and notAfter as we dont use them?
maybe even remove/makeOptional notBeforeInt, because emailAttestation issued on the attestation.id backend and must be validated on blockchain or another backend server, so no chanse to make attestation for future use?
attestation.id is using notBefore and notAfter. But this can be changed easily.
Personally, I don't think we have to remove them, because of backward compatibility. Even if we remove them, but the old attestations issued still have to be supported: the newest email attestation has 30 days time limit.
So, only schema can be changed and the validate code cannot be changed too. Even more, the old attestations are saved on chain, for some reasons, it still might be used or decoded. So, the old code still can not be removed.
At the same time, removing these fields can not bring too many advantages. Another example is API docs not changing those interfaces including typo aren't uncommon.
If we insist on removing, I suggest making a new version of schema, not changing on the same structure.
If we remove them now, I think we need to first change the default issuance structure to issuer attestation that only use notBeforeInt and notAfterInt. Then after that is deployed, we wait a month and finally remove the GeneralizedTime code.