-
Notifications
You must be signed in to change notification settings - Fork 16
Add support for V1 third party blocks #126
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
Add support for V1 third party blocks #126
Conversation
|
I would also update the samples from biscuit and add some unit test since I realize they are sadly missing. |
Agreed. It's on my list. I think I might not be able to do all of them because of not-yet-supported 3.3 things but I'll see what I can do. |
978290b to
453023e
Compare
samples numbered 24, 26, 36 and 37 should work without requiring any other 3.3 feature (generally the samples have been designed for testing incremental support, and the 3rd party blocks don’t use any otherwise fancy datalog feature) |
sounds good. I'm going to update as many of them as I can. There are some changes in the error messages too to sort out. |
This is clearly the idea. Take the latest version of the one present and add the unit tests. The new one will be in sync with the datalog function. |
| public static final int DATALOG_3_3_SIGNATURE_VERSION = 1; | ||
| public static final int NON_ED25519_SIGNATURE_VERSION = 1; | ||
|
|
||
| private static final byte[] BLOCK_VERSION = "\0BLOCK\0\0VERSION\0".getBytes(); |
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 force the Output Charset .getBytes(StandardCharsets.US_ASCII)
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 use the latest version from the biscuit repo
453023e to
2877bec
Compare
|
Updated all of the samples to latest, except for 3, 13, 17, 25, 27-35, and 38 which will require behavioural changes. If possible, I'd like to address updating those and add many more unit tests throughout in subsequent PRs. |
| @Override | ||
| public String toString() { | ||
| return "ThirdPartyBlockRequest{previousKey=" + previousKey + '}'; | ||
| return "ThirdPartyBlockRequest{previousKey=" + previousSignature + '}'; |
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.
| return "ThirdPartyBlockRequest{previousKey=" + previousSignature + '}'; | |
| return "ThirdPartyBlockRequest{previousSignature=" + previousSignature + '}'; |
Korbik
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.
LGTM with the latest suggestion. It is just to avoid to out of sync. Good job.
Also, add logic to decide what version to use when adding new blocks to a biscuit. These changes are analogous to the rust implementation and prepares for the adoption of the new V1 protobuf.
Old biscuits can still be verified using the older format, but new ones will be generated using the new format.
Samples 3, 13, 17, 25, 27-35, and 38 remain unchanged; they contain implementation changes that will need to be addressed in subsequent commits.
2877bec to
6ce2cc5
Compare
Pushed changes. Should be good to merge now. Thanks for the review. |
This adds support for both V0 and V1 blocks, closely aligning with the rust implementation.