Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Avoid to keep PrivateKey in HederaWallet object #5

Closed
franfernandez20 opened this issue Oct 23, 2023 · 7 comments · May be fixed by #40
Closed

Avoid to keep PrivateKey in HederaWallet object #5

franfernandez20 opened this issue Oct 23, 2023 · 7 comments · May be fixed by #40
Assignees
@hgraphql
Labels
enhancement New feature or request
Milestone
3+ Wallets integr...

Comments

@franfernandez20
Copy link
Contributor

I propose changing the approach to sign transactions and messages. I would avoid storing the PrivateKey in an object and delegate its use to a signer element like Client or Wallet.

Example for transactions:

// [...]
await transaction.sign(this._privateKey)
// [...]

to

// [...]
await transaction.signWithOperator(this._client)
// [...]

Example for messages:

  public signMessages(bytes: string[]): HederaSignMessageResponse {
    return {
      signatures: bytes.map((bytes) => {
        const buf = Buffer.from(bytes, 'base64')
        this._client._operator?.transactionSigner(buf)
        const signedMessage = this._privateKey.sign(buf)
        return Buffer.from(signedMessage).toString('base64')
      }),
    }
  }

to

// [...]
 this._wallet = new Wallet(accountId, privateKey)
// [...]
  public signMessages(bytes: string[]): HederaSignMessageResponse {
    return {
      signatures: bytes.map((bytes) => {
        const buf = Buffer.from(bytes, 'base64')
        const signedMessage = await this._wallet.sign([buf])
        // or 
        // const signedMessage = await this._client.getOperator().transactionSigner(buf)
        return Buffer.from(signedMessage).toString('base64')
      }),
    }
  }
@hgraphql hgraphql self-assigned this Oct 23, 2023
@hgraphql
Copy link
Contributor

agreed. thanks for raising this issue

@Volind
Copy link

Volind commented Oct 24, 2023

Yes, let's pass a transaction signer callback instead
of a key, this will allow wallet app to use hardware wallet for signing.

@hgraphql hgraphql removed their assignment Oct 26, 2023
@hgraphql hgraphql added the enhancement New feature or request label Oct 31, 2023
@hgraphql hgraphql added this to the 3+ Wallets integrated milestone Oct 31, 2023
@hgraphql hgraphql assigned BeeJeeNinja and hgraphql and unassigned BeeJeeNinja Nov 6, 2023
@hgraphql hgraphql moved this from Todo to In Progress in Hedera <> WalletConnect Nov 10, 2023
@hgraphql
Copy link
Contributor

stubbed out implementation using Wallet from @hashgraph/sdk here. untested yet, but this should allow us to delegate key management

hedera-walletconnect-utils/src/lib/wallet/base.ts

Line 100 in c919efd

async signTransactionAndSend(_signedTransaction: string): Promise<number> {

@itsbrandondev
Copy link
Member

updated weekly goal for this issue @hgraphql

@hgraphql hgraphql linked a pull request Nov 15, 2023 that will close this issue
Release/hip 820 #40
Open
@hgraphql
Copy link
Contributor

@itsbrandondev sounds good

it's being address here

hedera-walletconnect-utils/src/lib/wallet/index.ts

Line 43 in f6f4b09

this.hederaWallet = new HederaWallet(accountId, privateKey, provider)

though it's hard to share / get a review until it's demonstrated in a demo

@hgraphql
Copy link
Contributor

new structure involves passing a signer / wallet

hedera-walletconnect-utils/src/lib/wallet/index.ts

Line 144 in c691b9f

signer: HederaWallet,

@hgraphql
Copy link
Contributor

marking as closed as the lib is now using the Wallet signer

@github-project-automation github-project-automation bot moved this from In Progress to Done in Hedera <> WalletConnect Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@hgraphql hgraphql

Labels
enhancement New feature or request
Projects
No open projects
Hedera <> WalletConnect
Status: Done
Milestone
3+ Wallets integrated
Development

Successfully merging a pull request may close this issue.

Release/hip 820 hgraph-io/hedera-walletconnect-utils
5 participants
@Volind @franfernandez20 @BeeJeeNinja @itsbrandondev @hgraphql