Skip to content

Feature/update contracts sdk with snickerdoodle wallet #1268

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

Open
wants to merge 42 commits into
base: feature/new-contracts-package
Choose a base branch
from

Conversation

seansing9
Copy link
Contributor

Release Notes

JIRA Link

Summary:

This PR updates the contracts-sdk to new functionalities of the SnickerdoodleWallet and Factory contracts.

@seansing9 seansing9 requested a review from SnickerChar October 8, 2024 18:47
@seansing9 seansing9 requested a review from a team as a code owner October 8, 2024 18:47
@@ -32,6 +32,14 @@ import {
SignerUnavailableError,
NobleED25519KeyPair,
ED25519PrivateKey,
P256PublicKeyComponent,
PasskeyId,
PasskeyPublicKeyPointX,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be P256PublicKeyPointX rather than passkey, keeping it consistent with the P256Signature stuff?


public parseRawP256PublicKey(
id: PasskeyId,
publicKeyArray: Uint8Array,
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now we have a PublicKey defined and used as a binary hex string; it might be better to make this take that object. A new P256PublicKey primitive would be really good. Also, for PasskeyId, is that really required? It's part of P256PublicKeyComponent but it's not used by this method; I think we could exclude it safely.

// Third value tells you the type of the next value which MUST be an integer (0x02) if this is a signature array
const metadataIndicatorByte = pubKeyView[2];
// Third byte MUST be equal to 48 if this is a legitimate public key array
console.assert(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make this method return a Result<> so that this error could be returned. Doesn't need to be a ResultAsync<>, but this error should be properly handled

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty file


// returns a 64-byte ArrayBuffer containing r and s concatenated together
public parseRawP256Signature(
signatureArray: ArrayBuffer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change the name and type to signature: P256Signature

import { JSONString } from "@snickerdoodlelabs/objects";
import { BytesLike } from "ethers";

export class AuthenticatorData {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a method in CryptoUtils parseClientDataJson(clientDataJSON: JSONString): Result<AuthenticatorData, whatever>

@@ -0,0 +1,5 @@
import { P256SignatureR, P256SignatureS } from "@objects/primitives/index.js";

export class P256SignatureComponent {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ParsedP256Signature, or just make it P256SignatureComponents

P256PublicKeyPointY,
} from "@objects/primitives/index.js";

export class P256PublicKeyComponent {
Copy link
Contributor

Choose a reason for hiding this comment

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

Name -=> P256PublicKeyComponents, drop the keyId

parseRawP256Signature(
signatureArray,
msgPayload,
): P256SignatureComponentArrayBuffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this return a P256SignatureComponents object, can be passed to the contract layer, make sure it takes strings not arrays

parseRawP256PublicKey(
publicKey: P256PublicKey,
): Result<
{ x: P256PublicKeyPointX; y: P256PublicKeyPointY },
Copy link
Contributor

Choose a reason for hiding this comment

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

Return P256PublicKeyComponents (minus the key ID)

>;

addP256KeyWithP256Key(
keyId: PasskeyId,
Copy link
Contributor

Choose a reason for hiding this comment

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

PasskeyId -> WebauthnCredentialId

Copy link
Contributor

@SnickerChar SnickerChar left a comment

Choose a reason for hiding this comment

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

Notes after discussion


addP256KeyWithP256Key(
keyId: WebauthnCredentialId,
authenticatorData: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Primitive type string AuthenticatorData

@@ -281,4 +312,11 @@ export class SnickerdoodleWalletContract
): SnickerdoodleWalletContractError {
return new SnickerdoodleWalletContractError(msg, e, transaction);
}

private isoBase64fromBuffer(
Copy link
Contributor

Choose a reason for hiding this comment

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

This method isn't needed now, should remove it, along with the import

): Result<string, InvalidParametersError> {
return ok(evmAccountAddress);
): Result<Uint8Array, InvalidParametersError> {
// Remove 0x, convert to Uint8Array
Copy link
Contributor

Choose a reason for hiding this comment

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

You should check this out, and make sure it works with both 0x123 and just 123. I think that .encode will handle both versions, you should test that.

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.

3 participants