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

Taylor/wallet stamper client #409

Merged
merged 19 commits into from
Nov 20, 2024
Merged

Conversation

taylorjdawson
Copy link
Collaborator

@taylorjdawson taylorjdawson commented Oct 24, 2024

Summary & Motivation

Changes

@turnkey/wallet-stamper

WalletStamper
  • Renamed recoverPublicKey to getPublicKey
    Reason: Improves clarity and consistency across wallet interfaces

  • Unified getPublicKey method signature to take no parameters
    Reason: The change in method signature for getPublicKey (removing parameters) was necessary to allow the TurnkeyWalletClient to access the public key without needing to provide any additional information. This simplifies the process of retrieving the public key for sub-organization creation as demonstrated in the example.

  • Moved getPublicKey to BaseWalletInterface
    Reason: Ensures all wallet types implement this essential method

  • Updated SolanaWalletInterface and EthereumWalletInterface
    Reason: Streamlines interfaces by removing redundant method declarations

  • Modified EthereumWallet implementation
    Reason: Aligns with new interface structure and utilizes signMessage for public key recovery

@turnkey/sdk-browser

TurnkeyWalletClient
  • Added new TurnkeyWalletClient to the @turnkey/sdk-browser
    Reason: Allows using the WalletStamper with the browser sdk

  • Added getPublicKey method to TurnkeyWalletClient
    Reason: Enables easy access to wallet public key for sub-organization creation and future authentication flows

  • Updated TurnkeyWalletClient to use new WalletInterface
    Reason: Ensures compatibility with the updated Wallet Stamper interfaces

AuthClient (new enum)
  • Introduced a new enum to track which client is authenticated (Passkey, Wallet, Iframe)
    Reason: To be stored in the new user session local storage object, to determine which client to return in the @turnkey/sdk-react
TurnkeyBrowserClient, TurnkeyIframeClient, TurnkeyPasskeyClient, TurnkeyWalletClient
  • Added a static authClient property to base TurnkeyBrowserClient to be used by the child classes.
    Reason: To track which client was used for the initial authentication, enabling proper client retrieval later

  • Set specific authClient values in each subclass
    Reason: To ensure each client type is correctly identified for authentication and retrieval purposes

UserSession interface
  • Added a new UserSession interface which is to be stored in local storage to track the authentication state of the user.
    Reason: To eliminate the need to store the write and read sessions separately. Improving clarity and maintainability.
  • Added authenticatedClient in the session object
    Reason: To store the authentication method used in the user's session data. Will be used in the @turnkey/sdk-react to determine which client to return.

This is the new UserSession object which gets stored in local storage:

export interface User {
  userId: string;
  username: string;
  organization: {
    organizationId: string;
    organizationName: string;
  };
  session:
    | {
        read?: ReadOnlySession;
        write?: ReadWriteSession;
        authenticatedClient: AuthClient;
      }
    | undefined;
}
StorageKeys enum
  • Added new versioned UserSession key: "@turnkey/session/v1" Added a new storage key to remove reliance on the legacy user session object with the goal of not breaking any existing implementations with past versions of the sdk.
    Reason: To facilitate future updates without breaking existing implementations and improve backward compatibility.
login and loginWithReadWriteSession methods
  • Updated to use the new authenticatedClient property when creating user sessions
    Reason: To properly track and store the authentication method used during login

How I Tested These Changes

Did you add a changeset?

If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run pnpm changeset. pnpm changeset will generate a file where you should write a human friendly message about the changes. Note how this (example) includes the package name (should be auto added by the command) along with the type of semver change (major.minor.patch) (which you should set).

These changes will be used at release time to determine what packages to publish and how to bump their version. For more context see this comment.

Copy link

codesandbox-ci bot commented Oct 24, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@taylorjdawson taylorjdawson marked this pull request as draft October 24, 2024 18:06
Copy link

socket-security bot commented Oct 25, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@changesets/[email protected] Transitive: environment, eval, filesystem, shell, unsafe +102 16 MB changesets-release-bot
npm/[email protected] None +1 265 kB junscuzzy
npm/[email protected] Transitive: environment, filesystem, network +25 23.8 MB awkweb, jmoxey

🚮 Removed packages: npm/@changesets/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

@taylorjdawson taylorjdawson force-pushed the taylor/wallet-stamper-client branch from 4f9febe to 3447581 Compare October 25, 2024 00:13
@taylorjdawson taylorjdawson marked this pull request as ready for review October 25, 2024 18:20
Copy link
Collaborator

@andrewkmin andrewkmin left a comment

Choose a reason for hiding this comment

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

loooove the progress!

Updated SolanaWalletInterface and EvmWalletInterface
Reason: Streamlines interfaces by removing redundant method declarations

Modified EthereumWallet implementation
Reason: Aligns with new interface structure and utilizes signMessage for public key recovery

should we consolidate on EVM or Ethereum, not both?

also, would you be able to add a changeset for all the package changes? feel free to make them major updates. It would be great to get some iterations on getting the changes articulated in the changelog :)

const passkeyClient = turnkey.passkeyClient();

// User will be prompted to login with their passkey
await passkeyClient.login();
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to describe what exactly login is doing? or perhaps this can wait for Omkar's work on documenting all these methods

cc @omkarshanbhag as some of these methods will be getting tweaked

packages/sdk-browser/README.md Outdated Show resolved Hide resolved
packages/sdk-browser/README.md Show resolved Hide resolved
packages/sdk-browser/README.md Outdated Show resolved Hide resolved
packages/sdk-browser/README.md Outdated Show resolved Hide resolved
packages/sdk-browser/src/sdk-client.ts Show resolved Hide resolved
packages/sdk-browser/src/sdk-client.ts Outdated Show resolved Hide resolved
packages/sdk-browser/src/storage.ts Outdated Show resolved Hide resolved
packages/wallet-stamper/README.md Show resolved Hide resolved
packages/wallet-stamper/src/__tests__/wallet-interfaces.ts Outdated Show resolved Hide resolved
@taylorjdawson taylorjdawson force-pushed the taylor/wallet-stamper-client branch from 8daddec to 77d0e7f Compare November 6, 2024 21:02
@@ -375,7 +434,7 @@ export class TurnkeyPasskeyClient extends TurnkeyBrowserClient {
expirationSeconds: string = DEFAULT_SESSION_EXPIRATION,
organizationId?: string
): Promise<ReadWriteSession> => {
const localStorageUser = await getStorageValue(StorageKeys.CurrentUser);
const localStorageUser = await getStorageValue(StorageKeys.UserSession);
userId = userId ?? localStorageUser?.userId;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andrewkmin should we make userId optional given that it could comes from the local storage similar to organizationId?

packages/crypto/src/crypto.ts Show resolved Hide resolved
packages/sdk-browser/README.md Outdated Show resolved Hide resolved
packages/sdk-browser/README.md Show resolved Hide resolved
packages/sdk-browser/package.json Outdated Show resolved Hide resolved
@@ -140,13 +151,13 @@ export class TurnkeyBrowserSDK {
*/
currentUserSession = async (): Promise<TurnkeyBrowserClient | undefined> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

now that I'm reading this back... I wonder if we should combine currentUserSession and getReadWriteSession. I'd imagine if a user somehow has both a read-only and read-write session active, when they call currentUserSession, they'd probably want the read-write session to be returned?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andrewkmin if we did this is your idea that we would return the TurnkeyIframeClient instead of the TurnkeyBrowserClient for read-write sessions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

mm yeah that's tricky, i suppose it would have to be in that case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we could abstract this in the react sdk

const { client } = useTurnkey()

we could make this client more intelligent by defaulting to creating a read/write session regardless of which client authenticates initially
and the user would have the option of downgrading or setting a default auth level for the client in the config

@andrewkmin andrewkmin force-pushed the taylor/wallet-stamper-client branch from 8fc4845 to 30789f0 Compare November 16, 2024 04:44
Copy link
Collaborator

@andrewkmin andrewkmin left a comment

Choose a reason for hiding this comment

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

wow a herculean effort 👏 just some relatively minor comments

package.json Outdated Show resolved Hide resolved
const publicKey = (wallet as SolanaWalletInterface)?.recoverPublicKey();
const publicKey = await wallet?.getPublicKey();
if (!publicKey) {
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

sort of a meta question but do you prefer using nulls or undefineds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Generally I'll use null to distinguish between uninitialized variables and values that have been intentionally set to an empty value

packages/sdk-react/package.json Outdated Show resolved Hide resolved
getActiveClient: () => Promise<TurnkeyBrowserClient | undefined>;
}

export const TurnkeyContext = createContext<TurnkeyClientType>({
client: undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is the active client, should we name it activeClient for consistency with getActiveClient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah that makes sense! I was thinking of simplifying the name to abstract away some of the complexity of the multiple clients

@@ -193,12 +167,16 @@ const whoami = await client.getWhoami({
organizationId: process.env.NEXT_PUBLIC_ORGANIZATION_ID,
});

// Now that we have the sub organization id, we can make requests using that sub org id
if (!whoami?.userId) {
// User does not yet have a sub organization, so we need to create one
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we add suborg creation code here?

@@ -1,6 +1,6 @@
{
"name": "@turnkey/wallet-stamper",
"version": "0.0.5",
"version": "1.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we make this initial launch as beta? i.e. 0.1.0? or are we comfortable full sending as 1.0.0? I'm fine with either; if we're feeling good about the integration with demo wallet, let's do 1.0.0 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I was on the fence between beta/major version, I decided the API feels solidified -- I have some ideas for future improvements but they change the core API

Copy link
Collaborator

Choose a reason for hiding this comment

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

lots of great goodies here within the inline code comments themselves. we could link to this specifically within the changelog

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call!

: await this.wallet.recoverPublicKey(payload, signature);
// For Ethereum, we need to recover the public key from the signature over the payload.
// This is because recovering the SECP256K1 public key requires a signed message.
// This avoids the an additional call to the wallet to get the public key.
Copy link
Collaborator

Choose a reason for hiding this comment

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

tiny nit: let's choose one of the / an

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you explain how we avoid the additional call? wouldn't we need an extra one regardless in order to get that public key?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With solana I can call getPublicKey without triggering a signature request but with ethereum we need to derive it from the signature, so instead of calling getPublicKey we derive it from the signature over the payload

Copy link
Collaborator

Choose a reason for hiding this comment

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

OH! right right, that's beautiful!

@taylorjdawson taylorjdawson merged commit 031d861 into main Nov 20, 2024
5 checks passed
@taylorjdawson taylorjdawson deleted the taylor/wallet-stamper-client branch November 20, 2024 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants