Deprecate EIP712 authentication provider #532

piotr-roslaniec · 2024-06-21T12:04:55Z

Type of PR:

Feature

Required reviews:

1

What this does:

Marks EIP712AuthProvider as deprecated

Issues fixed/closed:

Refers to https://github.com/nucypher/tdec/issues/178

Notes for reviewers:

Should we outright remove EIP712 support instead of deprecating it? If not, what is a good timeline to allow taco-web users to adjust?
Created Remove EIP712 authentication support #536

netlify · 2024-06-21T12:05:10Z

✅ Deploy Preview for taco-nft-demo ready!

Name	Link
🔨 Latest commit	`9c7fdcd`
🔍 Latest deploy log	https://app.netlify.com/sites/taco-nft-demo/deploys/66756c6a87ec520008488dcc
😎 Deploy Preview	https://deploy-preview-532--taco-nft-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-06-21T12:05:12Z

✅ Deploy Preview for taco-demo ready!

Name	Link
🔨 Latest commit	`9c7fdcd`
🔍 Latest deploy log	https://app.netlify.com/sites/taco-demo/deploys/66756c6abafeaf0008fcd4a3
😎 Deploy Preview	https://deploy-preview-532--taco-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

derekpierre

Should we outright remove EIP712 support instead of deprecating it? If not, what is a good timeline to allow taco-web users to adjust?

Since it was implicit before I think it can just be removed. The associated new context variables can also be removed because they were never publicly released.

I think (?) the concern you are raising is a question for nucypher code instead of taco-web because everything was so implicit in the past - see the notes for reviewers in nucypher/nucypher#3515.

Let me know if I'm missing something.

derekpierre · 2024-06-27T14:24:44Z

packages/taco-auth/src/types.ts

 export const EIP712_AUTH_METHOD = 'EIP712';
 export const EIP4361_AUTH_METHOD = 'EIP4361';

 export const USER_ADDRESS_PARAM_DEFAULT = ':userAddress';
+
+/**
+ * @deprecated Use USER_ADDRESS_PARAM_EIP4361 instead.


These context variables were never publicly released - just the epic, right? If so, then a removal can be done instead of deprecation.

They were never exposed to the taco API, but the values they represent were in use, hence they should be deprecated.

Not sure I follow. Who used them in a released version? AFAIK they were only used internally for examples, and only in the epic. Am I mistaken?

No, I think you are right. I think that was my mistake. I will fix it.

^ Is the goal to remove these additional context variables in a separate PR then?

Yes, there's a follow-up issue for that: #536

But these don't need to wait T+2 releases to be removed ... ?

derekpierre · 2024-06-27T14:26:07Z

packages/taco-auth/src/providers/eip712.ts

+    console.warn(
+      'DeprecationWarning: The default authentication provider, EIP712AuthProvider, is deprecated. ' +
+      'Please use EIP4361AuthProvider instead. Refer to the documentation for more details.'
+    );


Since the use EIP712 was implicit until now, is this necessary?

EIP712AuthProvider was not actually used/specified by anyone in prior releases. Can we simply remove it?

IMHO it doesn't matter if behavior is implicit or explicit, only whether the changes in the API are breaking, and this is a breaking change

I agree with the intention of the warning, but should we refrain from still calling it the "default"?

I'm not sure I follow. I believe this file/module and the class was only added as part of the unreleased epic (the epic we are merging this PR into), so why keep the file/module and the class that we no longer intend to use as part of that epic and was never released...

By "implicit" I mean that no user has ever specified or created a EIP712AuthProvider object, because it was never released. So there is no real "breaking" for this class/module since has never been released. The broader API (decrypt) will indeed have "breaking" changes but that is irrespective of this particular auth provider, or the scheme used for validating wallets.

It's possible that a warning could be used somewhere, but I don't think this is the spot, since it seems like this file/module should just be deleted.

We've released an API with a documented behavior (using EIP712 by default) and are now replacing that behavior with another (EIP4361). I don't know specifically how this change can affect our users (is it going to "break" anything) but changing the implicit (or explicit) behavior of an API is a breaking change (according to semver).

Deprecating EIP712 in T+1 major version will allow us to communicate this breaking change to users who are currently using EIP712. Again, I don't know whether it will actually break something, but it makes sense to announce it and only remove it in T+2 after this upcoming change is communicated.

I see. Ok, so the goal of keeping the file is more to keep this (EIP712) as an option (albeit a pending deprecating one), in case some adopter decides they want to use EIP712 instead of EIP4361 for :userAddress for now until we fully deprecate. That's the reason for keeping this module for now.

If we are affording that option, then the server-side will need to be adjusted to accept both EIP712 and EIP4361 for :userAddress and nucypher/nucypher#3515 will need to be adjusted.

So the server side verification will be broken for EIP712 in the next release? Not deprecated, but outright removed?

In that case, these changes have no sense and we should remove EIP712 from the client too. I will address it in this PR.

So the server side verification will be broken for EIP712 in the next release? Not deprecated, but outright removed?

That's still a discussion for #3515, and not currently set in stone. We still need to make an official decision there.

piotr-roslaniec · 2024-07-02T09:16:40Z

Accidentally closed during a rebase

piotr-roslaniec changed the base branch from main to epic-auth June 21, 2024 12:05

piotr-roslaniec marked this pull request as ready for review June 24, 2024 12:38

piotr-roslaniec requested review from KPrasch, cygnusv, theref, derekpierre and manumonti June 24, 2024 12:39

piotr-roslaniec mentioned this pull request Jun 25, 2024

Expose auth providers from taco #534

Merged

piotr-roslaniec force-pushed the deprecate-eip712 branch from 9c7fdcd to 37cbd5d Compare June 26, 2024 12:55

derekpierre reviewed Jun 27, 2024

View reviewed changes

piotr-roslaniec force-pushed the deprecate-eip712 branch from 37cbd5d to c13f172 Compare June 28, 2024 09:01

piotr-roslaniec mentioned this pull request Jun 28, 2024

Remove EIP712 authentication support #536

Closed

piotr-roslaniec force-pushed the deprecate-eip712 branch 2 times, most recently from 517fb14 to 09f5732 Compare June 28, 2024 13:25

piotr-roslaniec mentioned this pull request Jul 1, 2024

[EPIC] Auth #527

Merged

17 tasks

piotr-roslaniec force-pushed the epic-auth branch from 4eea8e7 to 5a4d072 Compare July 2, 2024 08:40

piotr-roslaniec force-pushed the deprecate-eip712 branch from 09f5732 to ef0b40c Compare July 2, 2024 09:02

piotr-roslaniec closed this Jul 2, 2024

piotr-roslaniec force-pushed the deprecate-eip712 branch from ef0b40c to 5a4d072 Compare July 2, 2024 09:10

feat!(cond): deprecate eip712 auth provider

4fcb8ed

piotr-roslaniec reopened this Jul 2, 2024

cygnusv approved these changes Jul 2, 2024

View reviewed changes

piotr-roslaniec merged commit e9ce886 into nucypher:epic-auth Jul 2, 2024
8 checks passed

piotr-roslaniec deleted the deprecate-eip712 branch July 2, 2024 10:53

piotr-roslaniec mentioned this pull request Jul 4, 2024

Fast-track EIP712 deprecation #544

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate EIP712 authentication provider #532

Deprecate EIP712 authentication provider #532

piotr-roslaniec commented Jun 21, 2024 •

edited

Loading

netlify bot commented Jun 21, 2024 •

edited

Loading

netlify bot commented Jun 21, 2024 •

edited

Loading

derekpierre left a comment

derekpierre Jun 27, 2024

piotr-roslaniec Jun 28, 2024

derekpierre Jun 28, 2024

piotr-roslaniec Jun 28, 2024

derekpierre Jul 2, 2024

piotr-roslaniec Jul 2, 2024

derekpierre Jul 2, 2024

derekpierre Jun 27, 2024

piotr-roslaniec Jun 28, 2024

cygnusv Jun 28, 2024

derekpierre Jun 28, 2024

piotr-roslaniec Jun 28, 2024

derekpierre Jun 28, 2024 •

edited

Loading

piotr-roslaniec Jun 28, 2024

derekpierre Jun 28, 2024 •

edited

Loading

piotr-roslaniec commented Jul 2, 2024

Deprecate EIP712 authentication provider #532

Deprecate EIP712 authentication provider #532

Conversation

piotr-roslaniec commented Jun 21, 2024 • edited Loading

netlify bot commented Jun 21, 2024 • edited Loading

✅ Deploy Preview for taco-nft-demo ready!

netlify bot commented Jun 21, 2024 • edited Loading

✅ Deploy Preview for taco-demo ready!

derekpierre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

piotr-roslaniec commented Jul 2, 2024

piotr-roslaniec commented Jun 21, 2024 •

edited

Loading

netlify bot commented Jun 21, 2024 •

edited

Loading

netlify bot commented Jun 21, 2024 •

edited

Loading

derekpierre Jun 28, 2024 •

edited

Loading

derekpierre Jun 28, 2024 •

edited

Loading