-
Notifications
You must be signed in to change notification settings - Fork 11
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
Resolve CWT tagged and COSE-untagged messages #22
Conversation
Codecov Report
@@ Coverage Diff @@
## main #22 +/- ##
==========================================
+ Coverage 82.24% 82.39% +0.15%
==========================================
Files 10 10
Lines 1042 1051 +9
==========================================
+ Hits 857 866 +9
Misses 185 185
Continue to review full report at Codecov.
|
Found this one eu-digital-green-certificates/dgc-testdata#92
|
I am in favour of merging this (since we found some test data already that requires these changes). If we want to take a more conservative approach, we could wait for #7 to be completed just to have a better feeling for what's the percentage of the test data that comes without a CWT tag... |
This can be a good idea indeed. Moreover, we could also wait for #2 to be closed, in order to have even better overview of the unagged cases. Just a note: this change allows for messages tagged with CBOR Web token and then with the normal COSE tag. Did you find any discussion about this? |
Based on what I have seen in #23 I would get this one shipped. On this point:
I haven't seen the normal COSE tag in the test data. I am tempted to suggest to remove the support for it and keep the code simple... What do you think @dodomorandi? |
My fault, I've been lazy: for normal tag I meant the usual To be honest, I am extremely doubtful about the validity of that test, mostly because the official validator goes crazy as well with that. If you think that it is better for now to avoid that strange edge case, I can remove the code to handle it and keep things simpler. |
I am a bit conflicted as well, but if we can support an additional test case maybe it's best to ship the feature as it is. I think the additional level of complexity is acceptable... |
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 be happy to get this one merged :)
This makes a few more tests pass. Co-authored-by: Tommaso Allevi <tomallevi@gmail.com>
bdc3efc
to
54dfae9
Compare
I think we should discuss if this is a valid change or not. If we assume the given files are compliant, then it is ok. Otherwise this PR shall not be merged.
Open questions