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.

Sign up for GitHub

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

Jump to bottom

SQC-153: Support for verifyOwner #320

Closed
wants to merge 39 commits into from
Closed

SQC-153: Support for verifyOwner #320

wants to merge 39 commits into from

Conversation

lewis-sqa
Copy link
Contributor

@lewis-sqa lewis-sqa commented Jun 13, 2022 •
edited by kujtimprenkuSQA
Loading

Description

  • Added support for verifyOwner to all wallet integrations.
    • Sender exposes an empty Signer interface when locked.
    • Math Wallet hangs when calling signMessage - presumably it has some logic that detects if the message is a Transaction and discards anything else?
    • NEAR Wallet & My NEAR Wallet should use the new endpoint detailed here.

Note

This feature is only supported on MyNearWallet on testnet

Closes #318.

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.

  • 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.

@lewis-sqa lewis-sqa self-assigned this Jun 13, 2022
@lewis-sqa lewis-sqa changed the base branch from main to dev June 13, 2022 09:18
@lewis-sqa lewis-sqa mentioned this pull request Jun 15, 2022
Closed
@lewis-sqa lewis-sqa changed the title SQAC-167: Support for signMessage SQAC-167: Support for verifyOwner Jul 29, 2022
@MaximusHaximus MaximusHaximus changed the title SQAC-167: Support for verifyOwner SQC-153: Support for verifyOwner Aug 9, 2022
MaximusHaximus
MaximusHaximus suggested changes Aug 11, 2022
View reviewed changes
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

👍 Just one thing, in that Ledger is not supported currently; signing of messages other than transactions is not possible for the current ledger app.

packages/ledger/src/lib/ledger.ts
@@ -219,6 +220,41 @@ const Ledger: WalletBehaviourFactory<HardwareWallet> = async ({
return getAccounts();
},

async verifyOwner({ message = "verify owner", signerId, publicKey } = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently the NEAR Ledger app can only sign valid transactions; it will not be able to sign any payload that is not a transaction -- so this is not implemented currently and should be throwing a 'Method not Supported' type error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, for the feedback. I will make the changes tomorrow when I start working.

@MaximusHaximus MaximusHaximus marked this pull request as ready for review August 11, 2022 17:35
@MaximusHaximus
Copy link
Contributor

MaximusHaximus commented Aug 11, 2022 •
edited
Loading

@kujtimprenkuSQA Can you make the change to the ledger implementation to throw a method not implemented error before we try to connect to the ledger and request the public key from the device, and resolve the build check that started failing in 1a6e10f ? Once these are set I think this can be landed.

@kujtimprenkuSQA
Merge branch 'dev' into SQAC-167/sign-message-2
214ae8e 
# Conflicts:
#	packages/core/src/lib/wallet/wallet.types.ts
@kujtimprenkuSQA
Merge branch 'SQAC-167/sign-message-2' into SQAC-167/sign-message
d0f4e2d 
# Conflicts:
#	packages/core/src/lib/wallet/wallet.types.ts
@kujtimprenkuSQA
Copy link
Contributor

kujtimprenkuSQA commented Aug 11, 2022 •
edited
Loading

resolve the build check that started failing in 1a6e10f ? Once these are set I think this can be landed.

When I merged with the dev there were some changes to PULL_REQUEST_TEMPLATE during this merge and that's why the build is failing.
If I see that fixing this issue takes too much time, I will copy the work from here (about verifyOwner for each wallet) in a new PR, and make it ready for review.

kujtimprenkuSQA added a commit that referenced this pull request Aug 11, 2022
@kujtimprenkuSQA
Added verify owner method, copied code from PR #320
97dd9d6
@kujtimprenkuSQA kujtimprenkuSQA mentioned this pull request Aug 12, 2022
Merged
16 tasks
@kujtimprenkuSQA
Copy link
Contributor

Hey @MaximusHaximus I will be closing this PR because I was unable to quickly fix the build errors, I have copied all the code in new branch and addressed the requested changes in separate commits: #391

@gutsyphilip gutsyphilip mentioned this pull request Oct 12, 2022
Merged
@kujtimprenkuSQA kujtimprenkuSQA deleted the SQAC-167/sign-message branch August 15, 2023 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kujtimprenkuSQA kujtimprenkuSQA kujtimprenkuSQA left review comments

@MaximusHaximus MaximusHaximus MaximusHaximus requested changes

Assignees

@lewis-sqa lewis-sqa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Function to request wallet sign a message (a string, not a transaction).
3 participants
@lewis-sqa @MaximusHaximus @kujtimprenkuSQA