Seperate signer from provider in user-facing API
#270
Conversation
piotr-roslaniec
requested review from
KPrasch,
derekpierre,
manumonti,
jMyles,
cygnusv and
theref
piotr-roslaniec
force-pushed
the
sep-signer-provider
branch
from
2e7c3be
to
29fb44b
Compare
|
Bundled size for the package is listed below: build/main/src/characters: 74.22 KB |
Codecov Report
@@ Coverage Diff @@
## alpha #270 +/- ##
==========================================
+ Coverage 80.58% 80.63% +0.04%
==========================================
Files 36 36
Lines 994 1012 +18
Branches 108 112 +4
==========================================
+ Hits 801 816 +15
- Misses 185 188 +3
Partials 8 8
|
There was a problem hiding this comment.
Code looks good to me. 👍
About the convenience of separating Web3Provider into Provider and Signer, I would agree. For instance, recent versions of ethers have introduced this strong division (see "Coming from Web3.js" notice in https://docs.ethers.org/v5/api/providers/provider/#Provider).
IMHO this establishes a clear line between methods that require a provider with signer (like MetaMask does) and others that can use just a provider (like just using Infura or Alchemy endpoint). So, the developers have a cleaner and less confusing vision about what's going on under TACo/CBD.
I think that with some good examples in documentation, we can easily make clear for adopters how to get a signer from a provider (as Piotr mentioned).
Although maybe this is not the best place to discuss it, I would prefer not to choose the option of creating a SignerWithProvider object, since IMHO this add some complexity to the code, which can result in confusion. The terms Provider and Signer are pretty common in web3 development and I think most of the developers are familiar with them.
|
Thanks for the explanation. |
src/conditions/condition-expr.ts
Outdated
| @@ -85,9 +85,10 @@ export class ConditionExpression { | |||
| } | |||
|
|
|||
| public buildContext( | |||
| provider: ethers.providers.Web3Provider | |||
| provider: ethers.providers.Provider, | |||
| signer: ethers.Signer | |||
There was a problem hiding this comment.
Does buildContext get called if there are no context variables in the condition, eg. blocktime condition?
What if there are only user-defined context variables since they don't need anything signed?
Basically, I'm wondering if we properly handle signer being undefined if no signing is needed for the context...either because the condition doesn't need it (in which case buildContext may not even be called), or if there are only user-defined custom context variables in which case there is nothing to be signed.
There was a problem hiding this comment.
We could check ConditionExpression for the presence of context parameters and avoid passing signer in if it doesn't contain one. But we still need to make signer available in the user-facing API in case we need it (https://github.com/nucypher/nucypher-ts/blob/sep-signer-provider/src/characters/cbd-recipient.ts#L70)
I was considering this and other cases where the signer is optional. Which one of these approaches do we prefer?
- Make the signer optional and throw an error if it turns out we actually need it
- Split API into two user flows and make it required in one of them and remove it from the other.
I'm leaning towards the first option. I'm assuming that API users can detect/handle these cases correctly.
If we agree on 1), I will simply add optionality and relevant errors in this PR.
There was a problem hiding this comment.
I tentatively implemented these changes.
There was a problem hiding this comment.
I think 1) is reasonable.
I'm assuming that API users can detect/handle these cases correctly.
Is there a public API/utility function that can detect whether a signer is needed based on the condition?
There was a problem hiding this comment.
I think it's the ThresholdDecrypter.retrieveAndDecrypt, which will be exposed by taco.decrypt
There was a problem hiding this comment.
I guess I'm wondering, if I'm an API user,
- if a signer is needed and one is not provided I get prompted somehow
OR - do I need to inspect the condition myself to determine if
:userAddressis in it and therefore I know I need to provide a signer
OR - is there a function on a condition object, or the decrypter, or some other utility that provides me with a bool result as to whether I need the signer or not based on the condition
OR
...
TL;DR 😅 If signer is optional, how does the API user determine whether to pass it or not?
There was a problem hiding this comment.
if a signer is needed and one is not provided I get prompted somehow
There are only two ways that the user can learn that:
- From a function signature (it's "marked" as required, throws an error if missing during TS compilation etc.)
- From run-time errors - The parameter is marked as optional, but there is some dependency on it being present under certain conditions (context parameter is present or not). And because of this hidden dependency, we have to throw an exception in runtime instead
do I need to inspect the condition myself to determine if :userAddress is in it and therefore I know I need to provide a signer
If you want to avoid having errors thrown in runtime, then yes.
is there a function on a condition object, or the decrypter, or some other utility that provides me with a bool result as to whether I need the signer or not based on the condition
No, but we can add it.
There was a problem hiding this comment.
I've added a ConditionExpression.contextRequiresSigner helper function and related utilities. This is something end-users may use to check if they need to use a signer for their conditions (to build the context). We should document it an advanced usage guide.
piotr-roslaniec
force-pushed
the
sep-signer-provider
branch
from
eaafad0
to
0c28fa5
Compare
|
Bundled size for the package is listed below: build/main/src/characters: 74.22 KB |
| expect(conditionExpr.contextRequiresSigner()).toBe(false); | ||
| expect(conditionContext.requiresSigner()).toBe(false); |
There was a problem hiding this comment.
How come we need the check on both the ConditionExpr and the Context object? I would have assumed it's only needed in ConditionExpr.
There was a problem hiding this comment.
We don't necessarily need to check it - it's just when a wrapper for exposing this functionality is higher up in the API. I think it makes sense to add this check to the tests to document this API.
There was a problem hiding this comment.
I guess I really meant "expose" and not "check"
What's the case where the call requiresSigner() is performed on the Context instead of the Condition? Having it in the Context doesn't mesh with my understanding - the Context is just the dictionary of values, but it is the condition that determines whether a signer is needed or not.
There was a problem hiding this comment.
I think I had some use case in mind but on second thought it's already covered by ContextCondition.validate. I think we can make ContextCondition.requiresSigner private then.
There was a problem hiding this comment.
If still needed internally for validation - then yes, private would be better 👍 .
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers:
signer,provider). This is a mild inconvenience, and I could argue that we could simply take one of these instead and then check for the connection with the other. But in that case, we would have to throw an error when the user mishandles the API. We could also introduce our own type,SignerWithProvider, and use it where appropriate. IMHO these considerations are best left fortacoAPI, and not for this PR in particular.ethers-v5toviem#271 in the future