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

minor xdefi improvements #423

Merged
merged 5 commits into from
Mar 24, 2022
Merged

minor xdefi improvements #423

merged 5 commits into from
Mar 24, 2022

Conversation

mrnerdhair
Copy link
Contributor

@mrnerdhair mrnerdhair commented Feb 12, 2022

Requires #418 (not really, but otherwise I have to add a typecast and that would suck)

  • Fix some typos
  • Import less of lodash
  • Improve integration test mocks
  • Remove unnecessary create()/info() functions before anyone can get around to using them

@vercel
Copy link

vercel bot commented Feb 12, 2022

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/CdH9Be4HabEE3DEFkeQTEBWs8RVM
✅ Preview: https://hdwallet-git-xdefi-tweaks-shapeshift.vercel.app

packages/hdwallet-keepkey/src/adapter.ts Outdated Show resolved Hide resolved
@mrnerdhair mrnerdhair requested a review from cjthompson March 4, 2022 21:39
@mrnerdhair mrnerdhair added backwards-compatible reopen-later Stuff that's good to go but not a priority at the moment. labels Mar 4, 2022
@mrnerdhair mrnerdhair closed this Mar 5, 2022
@mrnerdhair mrnerdhair added this to the Low Priority milestone Mar 5, 2022
@mrnerdhair mrnerdhair reopened this Mar 14, 2022
@mrnerdhair mrnerdhair closed this Mar 14, 2022
@mrnerdhair mrnerdhair mentioned this pull request Mar 14, 2022
@mrnerdhair mrnerdhair removed backwards-compatible reopen-later Stuff that's good to go but not a priority at the moment. labels Mar 22, 2022
@mrnerdhair mrnerdhair reopened this Mar 22, 2022
@@ -11,7 +11,7 @@ class XDeFiTransport extends core.Transport {
}

export function isXDeFi(wallet: core.HDWallet): wallet is XDeFiHDWallet {
return _.isObject(wallet) && (wallet as any)._isXDeFi;
return isObject(wallet) && (wallet as any)._isXDeFi;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: If we're going to import some lodash util, might as well use _.has() which will handle the non-object cases

Copy link
Contributor Author

@mrnerdhair mrnerdhair Mar 23, 2022

Choose a reason for hiding this comment

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

Good point -- though IMO consistency with the rest of the wallets' implementations of this type of guard is valuable, and the endgame for them should just be to nuke them wholesale (see #466 / #435).

const wallet = createMockWallet();
if (!wallet) throw new Error("No XDeFi wallet found");
// mock xdefi
(globalThis as any).xfi = {
Copy link
Contributor

Choose a reason for hiding this comment

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

packages/hdwallet-xdefi/src/adapter.ts has the only occurrence of globalthis as any in the codebase currently - don't we have any way to avoid these two anys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can write type definitions for xdefi; explicit anys are prevalent in this repo, though, and I think it would make more sense to address them as a group. My plan is to get eslint in with the no-explicit-any rule turned off (#453) and then handle turning it on as a separate issue/pr.

@mrnerdhair mrnerdhair merged commit 803db1b into master Mar 24, 2022
@mrnerdhair mrnerdhair linked an issue Mar 25, 2022 that may be closed by this pull request
@mrnerdhair mrnerdhair removed this from the Low Priority milestone Apr 12, 2022
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.

minor xdefi issues
3 participants