Skip to content

Conversation

EmmanouelaPothitou
Copy link
Contributor

@EmmanouelaPothitou EmmanouelaPothitou commented Aug 13, 2024

Description

Support a new strategy web3_coinbase_signature during sign in/up flows that uses the Coinbase Wallet.

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Copy link

changeset-bot bot commented Aug 13, 2024

⚠️ No Changeset found

Latest commit: ff6068b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@EmmanouelaPothitou
Copy link
Contributor Author

!snapshot

@clerk-cookie
Copy link
Collaborator

Hey @EmmanouelaPothitou - the snapshot version command generated the following package versions:

Package Version
@clerk/astro 1.0.11-snapshot.vaadf3ba
@clerk/backend 1.6.2-snapshot.vaadf3ba
@clerk/chrome-extension 1.1.13-snapshot.vaadf3ba
@clerk/clerk-js 5.14.0-snapshot.vaadf3ba
@clerk/elements 0.12.4-snapshot.vaadf3ba
@clerk/clerk-expo 2.1.0-snapshot.vaadf3ba
@clerk/express 0.0.27-snapshot.vaadf3ba
@clerk/fastify 1.0.29-snapshot.vaadf3ba
@clerk/localizations 2.5.7-snapshot.vaadf3ba
@clerk/nextjs 5.3.0-snapshot.vaadf3ba
@clerk/clerk-react 5.4.0-snapshot.vaadf3ba
@clerk/remix 4.2.13-snapshot.vaadf3ba
@clerk/clerk-sdk-node 5.0.26-snapshot.vaadf3ba
@clerk/shared 2.5.0-snapshot.vaadf3ba
@clerk/tanstack-start 0.2.0-snapshot.vaadf3ba
@clerk/testing 1.2.9-snapshot.vaadf3ba
@clerk/themes 2.1.19-snapshot.vaadf3ba
@clerk/types 4.13.0-snapshot.vaadf3ba

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/astro

npm i @clerk/astro@1.0.11-snapshot.vaadf3ba --save-exact

@clerk/backend

npm i @clerk/backend@1.6.2-snapshot.vaadf3ba --save-exact

@clerk/chrome-extension

npm i @clerk/chrome-extension@1.1.13-snapshot.vaadf3ba --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@5.14.0-snapshot.vaadf3ba --save-exact

@clerk/elements

npm i @clerk/elements@0.12.4-snapshot.vaadf3ba --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@2.1.0-snapshot.vaadf3ba --save-exact

@clerk/express

npm i @clerk/express@0.0.27-snapshot.vaadf3ba --save-exact

@clerk/fastify

npm i @clerk/fastify@1.0.29-snapshot.vaadf3ba --save-exact

@clerk/localizations

npm i @clerk/localizations@2.5.7-snapshot.vaadf3ba --save-exact

@clerk/nextjs

npm i @clerk/nextjs@5.3.0-snapshot.vaadf3ba --save-exact

@clerk/clerk-react

npm i @clerk/clerk-react@5.4.0-snapshot.vaadf3ba --save-exact

@clerk/remix

npm i @clerk/remix@4.2.13-snapshot.vaadf3ba --save-exact

@clerk/clerk-sdk-node

npm i @clerk/clerk-sdk-node@5.0.26-snapshot.vaadf3ba --save-exact

@clerk/shared

npm i @clerk/shared@2.5.0-snapshot.vaadf3ba --save-exact

@clerk/tanstack-start

npm i @clerk/tanstack-start@0.2.0-snapshot.vaadf3ba --save-exact

@clerk/testing

npm i @clerk/testing@1.2.9-snapshot.vaadf3ba --save-exact

@clerk/themes

npm i @clerk/themes@2.1.19-snapshot.vaadf3ba --save-exact

@clerk/types

npm i @clerk/types@4.13.0-snapshot.vaadf3ba --save-exact

@nikosdouvlis
Copy link
Member

@EmmanouelaPothitou 👋🏻
what's the status of this one?

@EmmanouelaPothitou EmmanouelaPothitou changed the title wip feat(clerk-js,types): Add web3_coinbase_signature strategy on sign in/up flows Aug 20, 2024
@EmmanouelaPothitou EmmanouelaPothitou marked this pull request as ready for review August 22, 2024 16:58
@EmmanouelaPothitou EmmanouelaPothitou force-pushed the haris/coinbase branch 2 times, most recently from ee19cb8 to 70afda5 Compare August 23, 2024 06:25
@chanioxaris chanioxaris requested review from a team, chanioxaris and panteliselef and removed request for panteliselef August 23, 2024 10:20
}
};

public authenticateWithWeb3 = async ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ As we are introducing a new generic authenticateWithWeb3() function, we should also use it on the existing authenticateWithMetamask() method in order to DRY to code and avoid duplicating the logic

Also are we looking to introduce the corresponding authenticateWithCoinbase() method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have 2 options here:

  1. Deprecate authenticateWithMetamask and recommend transitioning to authenticateWithWeb3.
  2. Retain authenticateWithMetamask, introduce authenticateWithCoinbase and optionally mark authenticateWithWeb3 as private.


prepareWeb3WalletVerification = (): Promise<SignUpResource> => {
return this.prepareVerification({ strategy: 'web3_metamask_signature' });
prepareWeb3WalletVerification = (strategy: Web3Strategy): Promise<SignUpResource> => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛔ Isn't this a breaking change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately yes, strategy needs to be optional and fallback to 'web3_metamask_signature'

attemptPhoneNumberVerification: (params: AttemptPhoneNumberVerificationParams) => Promise<SignUpResource>;

prepareWeb3WalletVerification: () => Promise<SignUpResource>;
prepareWeb3WalletVerification: (strategy: Web3Strategy) => Promise<SignUpResource>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛔ Isn't this a breaking change?


export type AttemptWeb3WalletVerificationParams = {
signature: string;
strategy: Web3Strategy;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛔ Same here about a breaking change

export interface AuthenticateWithWeb3Params {
identifier: string;
generateSignature: GenerateSignature;
provider: 'metamask' | 'coinbase';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛔ Breaking change here as well

Comment on lines +103 to +143
function getMetamaskProvider(): any {
// @ts-ignore
if (!global.ethereum) {
return undefined;
}

// @ts-ignore
if (global.ethereum.providers) {
// @ts-ignore
return global.ethereum.providers[1];
}

// @ts-ignore
if (!global.ethereum.isMetaMask) {
return undefined;
}

// @ts-ignore
return global.ethereum;
}

function getCoinbaseProvider(): any {
// @ts-ignore
if (!global.ethereum) {
return undefined;
}

// @ts-ignore
if (global.ethereum.providers) {
// @ts-ignore
return global.ethereum.providers[0];
}

// @ts-ignore
if (!global.ethereum.isCoinbaseWallet) {
return undefined;
}

// @ts-ignore
return global.ethereum;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 We should find a more fine-grained way to use the ethereum provider in case there are multiple injected (e.g. user has MetaMask and Coinbase Wallet extensions).

We also have a separate ticket to deal with this edge case

@EmmanouelaPothitou EmmanouelaPothitou force-pushed the haris/coinbase branch 3 times, most recently from 441e0db to 3b521d6 Compare August 27, 2024 17:31
@chanioxaris chanioxaris marked this pull request as draft August 28, 2024 11:17
@chanioxaris chanioxaris closed this Sep 2, 2024
@chanioxaris chanioxaris deleted the haris/coinbase branch September 2, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants