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

Create CryptoKey Lib for linked accounts #1498

Merged
merged 10 commits into from
Jan 31, 2025
Merged

Conversation

cb-jake
Copy link
Contributor

@cb-jake cb-jake commented Jan 31, 2025

Summary

This PR adds a new lib called crypto-key which creates a new account for creating sub accounts.

How did you test your changes?

Unit tests

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Jan 31, 2025

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@cb-jake cb-jake force-pushed the jake/crypto-key-module branch from 3e67aff to 8c71fc8 Compare January 31, 2025 04:51
@coinbase coinbase deleted a comment from FoxyTams73 Jan 31, 2025
};
};

return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we exporting the getSigner implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getCryptoKeyAccount is a 1:1 for the getSigner function. Just named it to be more explicit on the ck side

@@ -0,0 +1 @@
# crypto-key
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest having a minimal readme here, since devs who want to bring their own key implementation would use this as a reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good callout! Added some basic docs. Will update as I start consuming the package with some "how tos"

montycheese
montycheese previously approved these changes Jan 31, 2025
@cb-heimdall
Copy link
Collaborator

Review Error for montycheese @ 2025-01-31 18:42:03 UTC
User failed mfa authentication, see go/mfa-help

const sign = async (payload: Hex.Hex) => {
const { payload: message, metadata } = WebAuthnP256.getSignPayload({
challenge: payload,
origin: 'https://keys.coinbase.com',
Copy link
Contributor

Choose a reason for hiding this comment

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

is this something we need to allow overriding (e.g. via env) for local testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@montycheese Great question. I believe if we were using a passkey vs a cryptokey this would need configured to the apps origin

@cb-jake cb-jake requested a review from montycheese January 31, 2025 19:20
@cb-jake cb-jake merged commit 4e8009a into master Jan 31, 2025
7 checks passed
@cb-jake cb-jake deleted the jake/crypto-key-module branch January 31, 2025 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants