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

Add Sui Support #138

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add Sui Support #138

wants to merge 7 commits into from

Conversation

0xzoz
Copy link

@0xzoz 0xzoz commented Jan 13, 2023

[Add Sui Support]

Description

This was created due to an initial PR created here and the library no longer being supported

This PR is to add SUI wallet support. Sui is a new layer 1 blockchain developed by Mysten Labs, it is in the early stages of launching and is part of the Move ecosystem that originated from the development work around Diem created at Meta

The PR adds the pkh-sui package and additional changes to the cacao package to support it.

It is being added here as a draft in case the team wants to provide context or comments. Some unit testing is done but it is awaiting local development testing using Sui wallets and guidance around the addition of CAIP's for Sui network(see below)

How Has This Been Tested?

Local unit testing

  • run local unit test

Describe the tests that you ran to verify your changes. Provide instructions for reproduction.

  • [ X ] sui.test.ts in the respective packages (e.g. Add new tests to cover additional code)
  • Local dev testing - TBD

PR checklist

Before submitting this PR, please make sure:

  • I have tagged the relevant reviewers and interested parties - N/A
  • I have updated the READMEs of affected packages - N/A
  • I have made corresponding changes to the documentation - N/A

References:

Please list relevant documentation (e.g. tech specs, articles, related work etc.) relevant to this change, and note if the documentation has been updated.

This PR is dependent on the following CAIP's and will require guidance from the Sui team.

Copy link
Contributor

@zachferland zachferland left a comment

Choose a reason for hiding this comment

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

Overall looks good so far, would mostly need to further define CAIP specs, have others review and reflect those changes/specs here.

return Cacao.fromSiwSuiMessage(siwSuiMessage)
}

export async function getAccountId(suiProvider: any, address: string): Promise<AccountId> {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is not possible to request the network from the provider, then you can remove this function, and just use getAccountIdByNetwork, as long as most of the networks are covered. All these functions are just optional helpers for each chain depending on implementation/needs

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is possible. Will change this when I get clarity on the verifySuiSignature method alternatives.

}
}

export function verifySuiSignature(cacao: Cacao, options: VerifyOptions) {
Copy link
Author

Choose a reason for hiding this comment

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

@zachferland Ive been looking into this more and it appears there is not currently a method to get the public key from the sui address to verify the signature. Im looking for insight from you to see what you would consider the best method for approaching this problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

is secp256k1 not also supported? then ecrecover from signature can be used (like eth)

Copy link
Author

Choose a reason for hiding this comment

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

keys can be either ed25519, secp256k1, secp256r1

Copy link
Contributor

Choose a reason for hiding this comment

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

do wallets support all three? is one default? how does that work typically

if these messages could just use secp256k1, that would be great

Copy link
Author

@0xzoz 0xzoz Feb 16, 2023

Choose a reason for hiding this comment

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

Wallets can be one of the 3 and it is specified at account creation. It is set by a flag when the keypair is created programmatically.

@ben-passion-xyz
Copy link

Hi, is there any progress with Sui support? I'm considering using composedb with Sui chain. I may not be familiar with signing algorithms, but is there any further information I can learn or help with?

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.

None yet

3 participants