Skip to content
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

#7 Add support for V2 format #16

Merged

Conversation

NeilMadden
Copy link
Contributor

This pull request adds support for the libmacaroons V2 format (binary only, not JSON) as per issue #7 . There are some tests based on the test vectors in the libmacaroons source code.

Please review and let me know of any changes required. (e.g., you advertise support for Android, but I know very little about whether this code will automatically work there or not).

@coveralls
Copy link

coveralls commented Nov 27, 2019

Coverage Status

Coverage remained the same at ?% when pulling f40b80f on NeilMadden:feature/libmacaroons_v2_format into 7440f46 on nitram509:master.

@nitram509
Copy link
Owner

Hi Neil,

thank you very much for providing this PR.
I'm sorry, I needed some time to look into ... in general I appreciate your design choices
and that you added some tests.
I thought further, how to make a new release - 0.5.0 would be appropriate I think.
There are two things I would like to ask if you would consider some refactoring.

Regarding the Serializers, there's now un-symmetry between V1 and V2.
I would rather see some breaking API changes in order to make this more consistent.
E.g. make MacaroonsSerializer the overarching interface
and just simply name the two implementation classes "MacaroonsSerializerV1" and "MacaroonsSerializerV2" so they both contain their counterparts of the serialization logic.

Also I very much like your idea of referring/copying test cases from libmacaroons.
It took a while for me to understand the test and I thought it would be better, to copy the original unit test cases from https://github.com/rescrv/libmacaroons/tree/master/test/unit as files.
I mean it would be great to have a (helper-)class to be able to load these test cases (or just selected ones) and fire the tests likewise. Then we would have way better coverage, the librarieries are interchangeable.
May I ask you to adopt the PR in such a way?

PS: I'm open to any feedback on the ideas and would very much appreciate your support because my current spare time is very limited due to the fact I'm strictly focussing on other projects.

The logic that was in MacaroonsSerializer/MacaroonsDeSerializer is now
in MacaroonsSerializerV1. MacaroonsSerializer is now the overall
interface, while MacaroonsDeSerializer has been deleted. The unit tests
of those classes have been merged into MacaroonsSerializerV1Test.
@NeilMadden
Copy link
Contributor Author

I've updated the PR to use the MacaroonsSerializer and MacaroonsSerializerV1/V2 as you say. I've not updated the tests to read the files from libmacaroons as there are only 3 serialization tests, but I can do this if it's a blocker for you. Parsing the other unit test files is more work and I'm not sure the tests add enough value for me to spend much time on them.

@nitram509
Copy link
Owner

Hi Neil,

thanks a lot for your update.
Rewriting the tests is no blocker for me. I'm thinking about your thoughts about the value, they will add and might decide later to do this.

I will add your PR and add add your Github Name and URL to the contributors list in the POM as well.
👍 thanks again for your contribution.

@nitram509 nitram509 merged commit f0bf48a into nitram509:master Dec 5, 2019
@NeilMadden
Copy link
Contributor Author

Glad to help.

@NeilMadden NeilMadden deleted the feature/libmacaroons_v2_format branch December 5, 2019 09:34
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.

3 participants