instructure / paseto

A paseto implementation in rust.
MIT License
151 stars 14 forks source link

Avoid taking ownership in the builder #18

Closed vbfox closed 4 years ago

vbfox commented 4 years ago

This change make the builders only borrow things and never take ownership.

It also make all methods of the builder take &mut self allowing a lot more flexibility as described in https://doc.rust-lang.org/1.0.0/style/ownership/builders.html

Motivation

Trying to use the builders I remarked that I had to re-parse the key each time when using v2 public mode as Ed25519KeyPair can't be copied. Digging in the code I remarked that there was quite a few other parts of the builder that were forcing copies where they only needed a borrow so I fixed them too.

Test Plan

I rand the tests after adapting them (Only a few changes needed) and they succeed.

Next Steps

⚠ This PR changes the API so it's a major semver change I suspect⚠

Mythra commented 4 years ago

Hey thank you for this PR! In general I think is a much better approach, for a builder. One change that I will ask for is adding a changelog entry to the CHANGELOG.md file. But I'd also like to ask if we can wait on this?

Specifically in the past we've used the patch version number only for our changes. We've reserved the major and minor version numbers for the actual specification we're based on (so it's quick to tell at a glance) what version of the specification we support. Unfortunately this means we've essentially recommended to users to match semver explicitly which is an unfortunate stance. I mean as many problems as semver has, right now we're even worse than that.

I'd like to take the time, and figure out a more sane version strategy, and publish a document on it as opposed to merging this right away, using the same version strategy we've always used, bump the patch version, and merge a better versioning scheme later.

If you need this PR we can merge it using the same versioning strategy we've always used, but if you're okay with waiting I'd like to figure out this versioning scheme sooner rather than later.

vbfox commented 4 years ago

I'll do a commit to update the changelog.

There is no need for an immediate release, I can live with pulling from my fork until things have stabilized. Also If I find the time I might send a few other PRs that'll change the public interface of the library (kinda like this one) so there might be more breaking changes that should all be released in one go.

For the versioning I see the problem, i'm not sure following the specification version is necessary here as the tokens themselves have a version embedded and the go version has already gone to 2.0 but ¯\_(ツ)_/¯

Mythra commented 4 years ago

Hey @vbfox ,

Can we rebase this one, or does your other PR need to be merged first?

vbfox commented 4 years ago

I rebased it

Mythra commented 4 years ago

Thanks again for this PR!