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 signTransaction() and signDelegateAction() to BaseWalletBehaviour interface #1213

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

gtsonevv
Copy link
Collaborator

@gtsonevv gtsonevv commented Oct 1, 2024

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

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.

@gtsonevv gtsonevv requested a review from frol October 1, 2024 12:33
@github-actions github-actions bot changed the title Add signTransaction() to BaseWalletBehaviour interface feat: Add signTransaction() to BaseWalletBehaviour interface Oct 1, 2024
@github-actions github-actions bot changed the title Add signTransaction() to BaseWalletBehaviour interface feat: Add signTransaction() to BaseWalletBehaviour interface Oct 1, 2024
@gtsonevv gtsonevv changed the title feat: Add signTransaction() to BaseWalletBehaviour interface feat: Add signTransaction() and signDelegateAction() to BaseWalletBehaviour interface Oct 16, 2024
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

So far it looks totally pointless. Do you have a proof of concept? We need a dApp with these interfaces used and miniwallet that will handle these operations.

What you also should think about is how dApp will receive the signed transaction from:

  1. web wallet - given that currently transaction signing experience redirects dApp user to a different website which redirects them back, so Promise is not going to work well here - we need to consider if we can open a new window instead of a redirect, and how that window will report the signed transaction back

  2. mobile wallet on the same device (when Wallet Selector-enabled website is used on the mobile device where I have the wallet app) - it will open a separate app, which somehow should give the control back

  3. mobile wallet for a desktop dApp - users are asked to scan a QR code, but again dApp needs to get a response back somehow

  4. browser extension

  5. Ledger device

Comment on lines 136 to 139
/**
* The {Signer} object that assists with signing keys
*/
signer: Signer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do you envision the use of it? Isn't the whole point to use an external wallet as signer? How is it required?

Comment on lines 132 to 135
/**
* The Transaction object to sign
*/
transaction: Tx;
Copy link
Collaborator

Choose a reason for hiding this comment

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

App developer in most of the cases cannot construct the full transaction since there is no public key and therefore there is no nonce. Also, it is often no account id either.

I cannot think of the use for SignTransactionParams interface as it is defined right now.

Comment on lines 155 to 158
/**
* Tx nonce.
*/
nonce: bigint;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tx nonce is attached to the public key nonce of the account, it does not make sense to require it from the app developer.

Comment on lines +210 to +213
/**
* Public key for the access key used to sign the delegate action
*/
publicKey: PublicKey;
Copy link
Collaborator

Choose a reason for hiding this comment

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

App developer does not have the user's public key

Comment on lines +186 to +189
/**
* Account ID for the intended signer of the delegate action
*/
senderId: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

App developer does not know the senderId

Comment on lines +202 to +205
/**
* Current nonce on the access key used to sign the delegate action
*/
nonce: bigint;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same problem as with transactions, see above

@Emadalshamery Emadalshamery mentioned this pull request Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: NEW❗
Development

Successfully merging this pull request may close these issues.

2 participants