-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: dev
Are you sure you want to change the base?
Conversation
// 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> { |
There was a problem hiding this comment.
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.
} | ||
|
||
protected async _getChainId(network: string): Promise<number> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpgundrum Please fix this.
There was a problem hiding this comment.
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];
// 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}); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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, | ||
} | ||
|
There was a problem hiding this comment.
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
Allows for users to bind their SS58 wallet address to a newly created Ethereum H160 address using the addressUnification pallet offered through peaq.