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

feat: Add Nightly Wallet to Selector #314

Merged
merged 12 commits into from
Jun 14, 2022
Merged

feat: Add Nightly Wallet to Selector #314

merged 12 commits into from
Jun 14, 2022

Conversation

NorbertBodziony
Copy link
Contributor

@NorbertBodziony NorbertBodziony commented Jun 1, 2022

Description

This PR adds support for Nightly extension wallet. It works with Solana and Near and has hardware wallet support.
I have added small changes to react template like eager connect for nightly and I think there is bug when wallet is already selected so also fixed that.

Closes # (issue)

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Type of change. This type of change is the main reason for the PR.

  • FIX - a PR of this type patches a bug.
  • FEATURE - a PR of this type introduces a new feature.
  • BUILD - a PR of this type introduces build changes.
  • CI - a PR of this type introduces CI changes.
  • DOCS - a PR of this type introduces DOCS improvement.
  • STYLE - a PR of this type introduces style changes.
  • REFACTOR - a PR of this type introduces refactoring.
  • PERFORMANCE - a PR of this type introduces performance changes.
  • TEST - a PR of this type adds more tests.
  • CHORE - a PR introduces other changes than the specified above.

@github-actions github-actions bot changed the title Feat: Add Nightly Wallet to Selector feat: Add Nightly Wallet to Selector Jun 1, 2022
Copy link

@wojciech-cichocki wojciech-cichocki left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kujtimprenkuSQA kujtimprenkuSQA left a comment

Choose a reason for hiding this comment

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

Please update the main readme to include Nightly wallet too.

packages/modal-ui/src/lib/components/WalletOptions.tsx Outdated Show resolved Hide resolved
examples/react/src/components/Content.tsx Outdated Show resolved Hide resolved
packages/nightly/src/lib/nightly.ts Outdated Show resolved Hide resolved
@NorbertBodziony
Copy link
Contributor Author

I have moved eager connect to an internal part of wallet, so front end users will not have to do anything.

Copy link
Contributor

@lewis-sqa lewis-sqa left a comment

Choose a reason for hiding this comment

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

Few comments from me 👍

I'm having a lot of difficulty importing my wallet using a seed phrase. I think the key pair is resolving correctly but it's lacking a lookup (or sometimes manually requested in some wallets during import) for the accountId. When I attempt to sign a transaction using this demo: https://near-template.vercel.app/, it's hits an error attempting to get the access key information (presumably for the nonce?) as it's passing a long string of characters instead of my testnet NEAR accountId

packages/nightly/src/lib/nightly.ts Outdated Show resolved Hide resolved
packages/nightly/src/lib/nightly.ts Outdated Show resolved Hide resolved
packages/nightly/src/lib/nightly.ts Outdated Show resolved Hide resolved
packages/nightly/src/lib/nightly.ts Outdated Show resolved Hide resolved
@NorbertBodziony
Copy link
Contributor Author

NorbertBodziony commented Jun 6, 2022

I'm having a lot of difficulty importing my wallet using a seed phrase. I think the key pair is resolving correctly but it's lacking a lookup (or sometimes manually requested in some wallets during import) for the accountId. When I attempt to sign a transaction using this demo: https://near-template.vercel.app/, it's hits an error attempting to get the access key information (presumably for the nonce?) as it's passing a long string of characters instead of my testnet NEAR accountId

We use implicit addresses for now we are going to work on custom ones in later releases. Demo only works if you already have some Near on your account since without it, you can't query it.

@NorbertBodziony
Copy link
Contributor Author

Fixed most of your comments. Eager connect requires update of extension it will take a couple of days cuz of Chrome.
Right now everything in terms of behaviour should be in line with other extensions like Sender. The only difference is that we show extension even if the user does not have this installed.

@NorbertBodziony
Copy link
Contributor Author

Extension with fix for eager connect just hit store. @lewis-sqa

@lewis-sqa
Copy link
Contributor

We use implicit addresses for now we are going to work on custom ones in later releases. Demo only works if you already have some Near on your account since without it, you can't query it.

Not sure I fully understand the last part of this. When I import my existing NEAR account that has some NEAR on it, it resolves to an incorrect account id. From what I understand, Nightly is correctly resolving my seed phrase in terms of the key pair, but deriving an implicit account id instead of linking to my .testnet account.

I've since created a new account directly through Nightly and sent some NEAR to it so I can test this extension. Are there plans to resolve this issue soon as I can foresee a lot of users having issues importing just like our team has?

packages/nightly/src/lib/injected-nightly.ts Outdated Show resolved Hide resolved
packages/nightly/src/lib/nightly.ts Outdated Show resolved Hide resolved
@NorbertBodziony
Copy link
Contributor Author

I've since created a new account directly through Nightly and sent some NEAR to it so I can test this extension. Are there plans to resolve this issue soon as I can foresee a lot of users having issues importing just like our team has?

Yes we are going to work on this, but we are still working on best possible UX right now we just use implicit address.
It also does not require you to deposit any near and is much easier for folks coming from ETH/SOL

Copy link
Contributor

@lewis-sqa lewis-sqa left a comment

Choose a reason for hiding this comment

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

Last few comments from me. There's also the change in WalletOptions.tsx that needs reverting

packages/nightly/src/lib/nightly.ts Outdated Show resolved Hide resolved
packages/nightly/src/lib/nightly.ts Outdated Show resolved Hide resolved
Comment on lines 76 to 83
const existingAccount = _state.wallet.account.accountId;

if (existingAccount) {
const nearAccount: Account = {
accountId: _state.wallet.account.accountId,
};
return [nearAccount];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should reuse getAccounts here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix 45125a1

@NorbertBodziony
Copy link
Contributor Author

Last few comments from me. There's also the change in WalletOptions.tsx that needs reverting

Removed

Copy link
Contributor

@lewis-sqa lewis-sqa left a comment

Choose a reason for hiding this comment

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

This can now be merged.

I should point out that during our testing of this wallet, we noticed the lock feature removes access to the account. The implication of this is Wallet Selector will consider Nightly to be signed out and unselect it as part of the setup logic when the dApp is refreshed for example.

Are there plans to make use of FunctionCall access keys with this wallet to enable "silent" signing of gas-only transactions?

@NorbertBodziony
Copy link
Contributor Author

Are there plans to make use of FunctionCall access keys with this wallet to enable "silent" signing of gas-only transactions?

Yes we want to connect this with custom AccountId support I don't have timeline for that, but we have that i our backlog.

@lewis-sqa lewis-sqa merged commit 60049ca into near:dev Jun 14, 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.

4 participants