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

feature/1208492315807986 Address unification in SDK #32

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

jpgundrum
Copy link
Contributor

@jpgundrum jpgundrum commented Oct 8, 2024

Allows for users to bind their SS58 wallet address to a newly created Ethereum H160 address using the addressUnification pallet offered through peaq.

@jpgundrum jpgundrum requested a review from irediaes October 8, 2024 23:17
// How does the user know what their mnenonmic phrase and private key are...
// - Can we safely return??

public async claimAccount(options: ClaimAccountOptions, statusCallback?: (result: ISubmittableResult) => void | Promise<void>): Promise<string> {
Copy link
Contributor Author

@jpgundrum jpgundrum Oct 8, 2024

Choose a reason for hiding this comment

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

Currently only offering address unification for newly created eth wallets which are mapped to a SS58 address.

packages/sdk/src/modules/unification/index.ts Outdated Show resolved Hide resolved
packages/sdk/src/modules/unification/index.ts Outdated Show resolved Hide resolved
@jpgundrum jpgundrum requested a review from irediaes October 22, 2024 15:20
packages/sdk/src/modules/unification/index.ts Show resolved Hide resolved
}

protected async _getChainId(network: string): Promise<number> {
Copy link
Member

Choose a reason for hiding this comment

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

The network param should be an enum type.

Copy link
Member

Choose a reason for hiding this comment

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

@jpgundrum Please fix this.

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 removed this function to only use the enum itself const chainId = ChainID[network.toUpperCase() as keyof typeof ChainID];

packages/sdk/src/modules/unification/index.ts Show resolved Hide resolved
@jpgundrum jpgundrum requested a review from irediaes December 3, 2024 19:30
// works when I create an ETH wallet from scratch that has no previous transactions on the network
// and when you first setup an initial keybind... make sure the baseUrl that you initialize to matches the network you are claiming the account with
it('known ss58 to newly created known h160 bind', async () => {
const result = await sdk.unification.claimAccount({network: "krest", substrateSeed: SEED, ethPrivate: ETH_PRIVATE});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@irediaes See how the use must manually set the network in the cmd. The backend code then checks the metadata api to see if the connected instance is on this same network.

Should we only use the metadata (initialized with sdk creation line 30) to get the chain id, or continue to use the network parameter that will be checked against the metadata?

const readable = properties.toHuman();

// want to make sure the set network is a known enum, and the set network is the same as the api metadata
if (ChainID[network.toUpperCase() as keyof typeof ChainID] == undefined
Copy link
Contributor Author

@jpgundrum jpgundrum Dec 3, 2024

Choose a reason for hiding this comment

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

This is where the check takes place. It first makes sure the network string is in the enum, and then compares to the currently connected api in the metadata to see if they align. Checking api instance before reduces amount of erroneous calls to chain.

KREST = 2241,
PEAQ = 3338,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Create new enum type to define network in the claim account options

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