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

Refactor ProofChain to use builder pattern #39

Open
zeeshanlakhani opened this issue Oct 26, 2022 · 8 comments
Open

Refactor ProofChain to use builder pattern #39

zeeshanlakhani opened this issue Oct 26, 2022 · 8 comments
Labels
enhancement New feature or request

Comments

@zeeshanlakhani
Copy link
Contributor

Note: This would be an API change

A Proofchain is always instantiated via ucan/cid and store, parser (to be soon replaced by the resolver). Upon further inspection, it seems better to use a builder pattern for this, where bare instantiation can follow from typical from/try_from methods, like so:

let chain = ProofChain::try_from(attenuated_token.as_str())
.unwrap()
.with_parser(did_parser)
.await
.unwrap()
.with_store(&store)
.build()
.await
.unwrap();

While somewhat more verbose in use, this allows for simpler recursion patterns, and seems to better associate store+parser w/ the proofchain created, for which they rely on.

Thoughts? PoC here: https://github.com/ucan-wg/rs-ucan/pull/38/files.

@cdata
Copy link
Member

cdata commented Nov 28, 2022

The builder pattern is most suitable in cases where the following conditions are met:

  • There are a lot of fields that are needed to initialize a struct
  • Some significant subset of those fields are either optional or have reasonable defaults

In this case, the builder pattern reduces verbosity for the user, sparing them from the mechanical task of filling out a bunch of implicit field values.

Can you go into more detail about how the builder pattern enables better recursion?

@zeeshanlakhani
Copy link
Contributor Author

@cdata for me, in looking at it, it was also the fact that a proofchain was dependent on the creation of a parser and store, and this PR makes that way more explicit, at least that's what I was going for. I initially did think about defaulting to the memory store for the backend, if that was considered a good move.

@zeeshanlakhani
Copy link
Contributor Author

zeeshanlakhani commented Nov 28, 2022

@cdata as far as the recursion goes, I think the build call is cleaner in this variant, as it pushes validation to the use of the parser, but you can look at the PR to see.

@zeeshanlakhani
Copy link
Contributor Author

@cdata the other case for the builder is when construction has side effects, and this makes explicit what the pieces are.

@cdata
Copy link
Member

cdata commented Nov 29, 2022

What utility does a user of this interface get from the intermediate stages of construction being exposed here?

@zeeshanlakhani
Copy link
Contributor Author

zeeshanlakhani commented Nov 29, 2022

@cdata mainly it was making the creation of a proof chain from a ucan string or CID more explicit in its reliance on the parser and store. I think there may be a better abstraction overall, so totally ok to let this go (minus some minor cleaned-up API bits in this PR), but in reading the code, proof chain creation from string/CID seemed to make the store and parser as implicit side effects.

@cdata
Copy link
Member

cdata commented Nov 29, 2022

in reading the code, proof chain creation from string/CID seemed to make the store and parser as implicit side effects.

I think you may be on to something, but I don't fully understand what you mean by this; are you saying that because you need these things in the invocation, you're implicitly requiring their instantiation at the time we construct a ProofChain?

FWIW in the practical usage within Noosphere, both the DidParser and the store implementation exist well in advance of this invocation, and aren't created on the fly.

@zeeshanlakhani
Copy link
Contributor Author

@cdata well, you can create the proofchain in parts, but even with the store and parser in advance, this concept still works. It's more that a proofchain builder makes the need for them to exist somewhere in the code extremely explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants