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

native: improve isolated crypto interfaces #366

Merged
merged 11 commits into from
Nov 12, 2021

Conversation

mrnerdhair
Copy link
Contributor

@mrnerdhair mrnerdhair commented Nov 4, 2021

This updates the isolated crypto interfaces so that all methods (and fields) are async. The previous definition supported an async implementation, but would have required a complicated wrapper to ensure that a couple of operations "looked" synchronous. Eliminating that requirement comes primarily from having touched enough of the code to realize that buildInput() can be made async safely.

This also updates all the native wallet so that it can be initialized with an Isolation.Core.Bip32.Node instead of an Isolation.Core.Bip32.Mnemonic, which permits simpler isolation engine implementations, and renames the Dummy engine to Default to better reflect its role and robustness.

@vercel
Copy link

vercel bot commented Nov 4, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shapeshift/hdwallet/DrP6CQH99qzCKZB6arbx4oQtdeyK
✅ Preview: https://hdwallet-git-improve-isolation-interface-shapeshift.vercel.app

@@ -215,10 +215,10 @@ export function MixinNativeBTCWallet<TBase extends core.Constructor<NativeHDWall
return true;
}

buildInput(coin: core.Coin, input: core.BTCSignTxInputNative): InputData | null {
return this.needsMnemonic(!!this.#seed, () => {
async buildInput(coin: core.Coin, input: core.BTCSignTxInputNative): Promise<InputData | null> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the core reason things weren't like this before -- because buildInput() was synchronous and technically part of the public interface. That had trickle-down effects that prevented util.getKeyPair() from being async and from there propagated out.

Now that I'm more comfortable that I have a full picture of the the overall library and its use cases, I'm confident that this method should be marked protected if it weren't for the fact that we're unable to do so because the mixin strategy we use is build on exporting class expressions, which doesn't support protected or private. Therefore, I don't think this counts as a modification to the public API, so we can change it without breaking things.

Comment on lines +6 to +7
export default {
async create(keyPair: BIP32.Node): Promise<SigningDelegate> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This helps normalize the way that isolation adapters are used -- via await Isolation.Adapters.Binance.create() just like any other isolation adapter.

@mrnerdhair mrnerdhair changed the title Improve isolation interface native: improve isolated crypto interfaces Nov 4, 2021
@mrnerdhair mrnerdhair force-pushed the improve-isolation-interface branch from c22f436 to 418e9c1 Compare November 4, 2021 18:10
@mrnerdhair mrnerdhair marked this pull request as ready for review November 4, 2021 18:12
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.

1 participant