Feat/dApp signer enhancement #76

franfernandez20 · 2024-03-09T14:52:42Z

Description:
General improvements to the DAppConnector. The enhancements cover various aspects of the connector, supporting several sessions, and facilitating the use of the client from the dApps side.

Add methods to directly use transaction
Support multiple sessions
Remove unnecessary code
Enhance certain performance aspects.

Related issue(s):

Fixes #

Notes for reviewer:

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

This commit includes general improvements to the DAppConnector. The enhancements cover various aspects of the connector, supporting several sessions, and facilitate the use of the client from the dApps side. Signed-off-by: Fran Fernandez <fran@kabila.app>

Signed-off-by: Fran Fernandez <fran@kabila.app>

README.md

Signed-off-by: Fran Fernandez <fran@kabila.app>

src/lib/dapp/index.ts

franfernandez20 · 2024-04-09T14:06:15Z

Hi @mgarbs, can I kindly request to have another reviewer assigned to this PR?

justynspooner

Nice work 👍 Looks like a good improvement for simplifying the setup for dapps.

The main thing that jumps out is the removal of the logic to test for active connections around persisted sessions.

Happy to look at approving once those are reviewed.

Thanks!

justynspooner · 2024-04-16T10:32:30Z

src/lib/dapp/index.ts

-            new Promise(async (resolve, reject) => {
-              try {
-                await this.pingWithRetry(session.topic)
-                resolve(session)
-              } catch (error) {
-                try {
-                  await this.walletConnectClient!.disconnect({
-                    topic: session.topic,
-                    reason: getSdkError('SESSION_SETTLEMENT_FAILED'),
-                  })
-                  reject(`Ping failed, disconnecting from session. Topic: ${session.topic}`)
-                } catch (e) {
-                  console.log('Non existing session with topic:', session.topic)
-                  reject('Non existing session')
-                }
-              }
-            }),


Are we confident that we don't need this logic anymore?

How do we clear out old sessions that no longer have an active connection?

Yes, the problem with that code is that it removes the session if the wallet is not replying. We have the disconnect or disconnectAll methods.

src/lib/dapp/index.ts

justynspooner · 2024-04-16T10:57:48Z

src/lib/dapp/index.ts

+    } else {
+      if (!transaction) throw new Error('Transaction is not defined')
+      const signer = this.getSigner(paramsOrAccountId as AccountId)
+      return signer.executeTransaction(transaction)
+    }


The else is redundant here (and in the similar transaction functions) since you're already returning if the above condition is true:

Suggested change

} else {

if (!transaction) throw new Error('Transaction is not defined')

const signer = this.getSigner(paramsOrAccountId as AccountId)

return signer.executeTransaction(transaction)

}

}

if (!transaction) throw new Error('Transaction is not defined')

const signer = this.getSigner(paramsOrAccountId as AccountId)

return signer.executeTransaction(transaction)

Completely agree with that @justynspooner but that is opinionable and a matter of preferences in the coding.

justynspooner · 2024-04-16T11:38:26Z

src/lib/dapp/index.ts

+    paramsOrAccountId: SignAndExecuteTransactionParams | AccountId,
+    transaction?: Transaction,
+  ) {
+    if (arguments.length === 1 && isSignAndExecuteTransactionParams(paramsOrAccountId)) {


Instead of manually creating guards we could probably improve all these checks and the error messages we return by using Zod

This would let us have the static types and the runtime checks bound to the same schema definition.

import { z } from 'zod'; // Create our Zod schema const SignAndExecuteTransactionParamsSchema = z.object({ signerAccountId: z.string(), transactionList: z.string(), }); // Infer the TypeScript types that we can use around our app type SignAndExecuteTransactionParams = z.infer<typeof SignAndExecuteTransactionParamsSchema>; // Runtime checks const result = SignAndExecuteTransactionParamsSchema.safeParse(paramsOrAccountId); if (!result.success) { result.error.issues; /* [ { "code": "invalid_type", "expected": "string", "received": "number", "path": [ "signerAccountId" ], "message": "Expected string, received number" } ] */ }

I don't suggest we need to add this into this PR but I'll leave here if someone (maybe me when I get some cycles) can add a new PR in the future.

I don't agree. I think the library should be as raw as possible. However, a new PR to discuss it would be great.

justynspooner · 2024-04-16T11:46:57Z

src/lib/dapp/DAppSigner.ts

@@ -89,15 +125,32 @@ export class DAppSigner implements Signer {
    throw new Error('Method not implemented.')
  }

-  async checkTransaction<T extends Transaction>(transaction: T): Promise<T> {
-    throw new Error('Method not implemented.')
+  async signTransaction<T extends Transaction>(transaction: T) {


Would be good to see some tests for this to improve confidence in its function.

Yes, absolutely. I did as much as I could. Let's unlock this contribution and we can continue improving it.

pwoosam · 2024-04-17T06:15:31Z

src/lib/dapp/DAppSigner.ts

@@ -106,4 +159,47 @@ export class DAppSigner implements Signer {
  ): Promise<OutputT> {
    throw new Error('Method not implemented.')


call needs to be implemented to support transaction.executeWithSigner(signer) https://github.com/hashgraph/hedera-sdk-js/blob/65a2323f8d9281d686c2f1c4d337b7a22a3ca97c/src/Executable.js#L500

Agree but as for the last comment. Let's unlock this contribution and we can continue improving it.

pwoosam · 2024-04-17T06:19:04Z

src/lib/dapp/DAppSigner.ts

@@ -106,4 +159,47 @@ export class DAppSigner implements Signer {
  ): Promise<OutputT> {
    throw new Error('Method not implemented.')
  }
+
+  /*
+   *  Extra methods


I see how these methods could be useful to dApp developers. But exposing these methods may back walletconnect into a corner where it's Signer implementation exposes extra methods that dApps become dependent. I think these extra methods should all be private, unless Signer also publicly exposes the same methods.

If you want to use separately the signer functionality from the DAppConnector and session manage it is mandatory to keep those methods public. Furthermore, there are no more issues to having them exposed than having nothing to use.

If you want to use separately the signer functionality from the DAppConnector and session manage it is mandatory to keep those methods public.

I'm not completely understanding this. Can you explain what you mean by use separately the signer functionality from the DAppConnector and session manage?

My understanding is that DAppConnector should be used to session manage and DAppSigner should be used as an implementation for the hashgraph/sdk's Signer interface. Right now DAppSigner is leaking its abstraction details to the user.

pwoosam · 2024-04-17T06:22:29Z

src/lib/dapp/DAppSigner.ts

-  ) {}
+  ) {
+    this.signerAccountId = `${ledgerIdToCAIPChainId(ledgerId)}:${accountId.toString()}`
+    this.nodesAccountIds = [AccountId.fromString('0.0.3')]


this.nodesAccountIds = [AccountId.fromString('0.0.3')] is a single point of failure on consensus node 3. Consider creating a hashgraph/sdk client and grabbing node ids from it's _network property. See this Signer implementation from the hashgraph/sdk https://github.com/hashgraph/hedera-sdk-js/blob/main/src/Wallet.js#L272

With the sdk client, you don't ensure you use the same nodes as the wallet you are using. So I think the best option is to set manually the available nodes after requesting them to the wallet. Anyway, I think here we have a good point to improve. I suggest creating a new PR to address the issue.

With the sdk client, you don't ensure you use the same nodes as the wallet you are using.

I don't think this should be a requirement for walletconnect. The nodeAccountIds on a transaction can differ from the nodes returned by a wallet, as long as they are nodes for the same network.

@franfernandez20 we absolutely cannot be hardcoding node id's like this, if node 3 goes down all transactions using this node will fail

To set the list, you can use the setNodes function. It is essential to ensure that the nodes in the client and wallet are aligned. Although we need to restore the Transaction.toBytes() approach to retain more nodes, we now have to use this method with only one node.

Anyway, what do you propose?

pwoosam · 2024-04-17T06:23:50Z

src/lib/dapp/DAppSigner.ts

+   * @param {AccountId[]} nodesAccountIds
+   */
+  setNodes(nodesAccountIds: AccountId[]): void {
+    this.nodesAccountIds = nodesAccountIds


Same comment. I think nodeAccountIds can be fetched more reliably using a Client or Provider instead of allowing the dApp to explicitly set them like this.

If the dApp wants to explicitly set nodeAccountIds, it should do so on the transaction itself instead of DAppSigner.

Same suggestion :)

pwoosam · 2024-04-17T06:28:28Z

src/lib/dapp/DAppSigner.ts

+    this.nodesAccountIds = [AccountId.fromString('0.0.3')]
+  }
+
+  static initialize(signClient: ISignClient) {


I think it was more elegant to pass the signClient in the constructor like it was before instead of using a static method to set a static prop that's shared between all instances.

That is debatable. For now, we have nothing to ease the dApp developers' integration. Let's start with this contribution and then discuss minor details like this.

pwoosam · 2024-04-17T06:39:55Z

src/lib/dapp/index.ts

+
+  public getSigner(accountId: AccountId): DAppSigner {
+    const signer = this.signers.find((signer) => signer.getAccountId().equals(accountId))
+    if (!signer) throw new Error('Signer is not found for this accountId')


nit: Returning undefined should be ok if not found. The dApp is likely better off handling an undefined result than try/catching this exception.

pwoosam · 2024-04-17T06:41:42Z

src/lib/dapp/DAppSigner.ts


 export class DAppSigner implements Signer {
+  static signClient: ISignClient
+  public signerAccountId: string


Should props that are not already exposed by the Signer interface be private?

pwoosam · 2024-04-17T06:44:36Z

src/lib/dapp/index.ts

+    } catch (error) {
+      throw error
+    } finally {
+      this.walletConnectModal.closeModal()


Excellent catch with this finally block!

Instead of being sarcastic and nitpicking, please be proactive and offer solutions. Also, it would be great if your comments included code solutions. Like other colleagues do.

I'm sorry for the misunderstanding :( I was genuine with this comment. When errors were thrown previously, the modal stayed open and was unusable. This was an excellent find.

pwoosam · 2024-04-17T06:54:12Z

src/lib/dapp/index.ts

+   * const result = await dAppConnector.executeTransaction(AccountId.fromString('0.0.12345), transaction)
+   * ```
+   */
+  public async executeTransaction(


I'm questioning if we should remove the execute*/sign* methods from the DAppConnector entirely and instead only use the Signer for executing transactions.

It feels like the DAppConnector should limit its responsibility managing walletconnect sessions and extending walletconnect.

I agree with that but I keep there to be backwards compatible with the previous versions.

Co-authored-by: Justyn Spooner <justynjj@gmail.com> Signed-off-by: Fran Fernández <franfernandez20@gmail.com>

pwoosam · 2024-04-17T15:30:01Z

@franfernandez20 I don't support merging this PR in its current state. The PR comments I left were marked resolved without any changes.

To unblock this PR, I think it should be split into at least 3 PRs with minimal changes:

DAppSigner enhancement
Close walletConnectModal on error
DAppConnector execution/sign method improvements

A PR should have a single, unique goal. This way contributors can agree on the minimal set of changes and focus discussion on a single topic to unblock the PR.

pwoosam · 2024-04-18T04:56:13Z

I decided to take a shot at the DAppSigner enhancement in another PR: #116

hgraphql · 2024-04-23T20:47:49Z

It looks like there's a bit of unresolved comments that have been marked resolved. Moving forward can we have both the commenter and the contributor agree when a conversation is resolved?

hgraphql · 2024-04-23T20:51:48Z

I decided to take a shot at the DAppSigner enhancement in another PR: #116

Should we view this as an alternate approach or a complementary PR?

Are there pieces that overlap?

We may want to consider a "release" branch so that we can begin to come to consensus on a more granular basis.

pwoosam · 2024-04-23T22:16:03Z

@hgraphql #116 is an alternate approach.

#116 implements more of Signer interface, enables transaction.executeWithSigner(signer) usage, adds unit tests for the signer, updates the example dApp have new sections to test the signer methods end to end, addresses the feedback in this PR, and adds support for some queries that return array results instead of individual objects.

pwoosam · 2024-04-23T22:22:17Z

There is significant overlap between the PRs, but #116 focuses on only what is needed to support implementing the Signer interface and testing it. #76 has a few other unrelated enhancements in it.

franfernandez20 · 2024-05-01T06:52:45Z

@franfernandez20 I don't support merging this PR in its current state. The PR comments I left were marked resolved without any changes.

To unblock this PR, I think it should be split into at least 3 PRs with minimal changes:

DAppSigner enhancement

Close walletConnectModal on error

DAppConnector execution/sign method improvements

A PR should have a single, unique goal. This way contributors can agree on the minimal set of changes and focus discussion on a single topic to unblock the PR.

Indeed, it would be preferable to have fewer PRs. However, we are starting from an unfinished version of this dAppSigner, which complicates the process of having one PR per topic, especially since there are more than three topics involved. While it might require more effort, I will attempt to manage it. Nonetheless, I believe it might be more efficient to first achieve a functional version and then focus on making improvements.

hgraphql · 2024-05-20T17:05:18Z

Great convos here!

Closing this PR in favor #116 , #134 , #135

If I'm mistaken on this feel free to re-open!

franfernandez20 and others added 3 commits March 9, 2024 02:20

Add DAppConnector usage documentation

639fddc

Signed-off-by: Fran Fernandez <fran@kabila.app>

Adapt DAppConnector for tests

d167d28

Signed-off-by: Fran Fernandez <fran@kabila.app>

franfernandez20 changed the title ~~Feat/d app signer enhancement~~ Feat/dApp signer enhancement Mar 9, 2024

zanctor reviewed Mar 19, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

franfernandez20 requested a review from zanctor March 26, 2024 15:51

fix: Typo in documentation

0e58895

Signed-off-by: Fran Fernandez <fran@kabila.app>

franfernandez20 force-pushed the feat/dAppSigner-enhancement branch from c305420 to 0e58895 Compare April 1, 2024 08:59

Merge branch 'main' into feat/dAppSigner-enhancement

d81d14f

zanctor reviewed Apr 4, 2024

View reviewed changes

src/lib/dapp/index.ts Show resolved Hide resolved

mgarbs requested review from justynspooner and bugbytesinc April 9, 2024 15:25

justynspooner reviewed Apr 16, 2024

View reviewed changes

pwoosam suggested changes Apr 17, 2024

View reviewed changes

Update src/lib/dapp/index.ts

56be968

Co-authored-by: Justyn Spooner <justynjj@gmail.com> Signed-off-by: Fran Fernández <franfernandez20@gmail.com>

franfernandez20 mentioned this pull request May 16, 2024

Simplify method in DAppSigner #134

Open

2 tasks

hgraphql closed this May 20, 2024

		@@ -106,4 +159,47 @@ export class DAppSigner implements Signer {
		): Promise<OutputT> {
		throw new Error('Method not implemented.')

Feat/dApp signer enhancement #76

Feat/dApp signer enhancement #76

Conversation

franfernandez20 commented Mar 9, 2024

franfernandez20 commented Apr 9, 2024

justynspooner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franfernandez20 Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwoosam Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwoosam commented Apr 17, 2024

pwoosam commented Apr 18, 2024

hgraphql commented Apr 23, 2024 • edited Loading

hgraphql commented Apr 23, 2024

pwoosam commented Apr 23, 2024

pwoosam commented Apr 23, 2024

franfernandez20 commented May 1, 2024

hgraphql commented May 20, 2024

franfernandez20 Apr 17, 2024 •

edited

Loading

pwoosam Apr 17, 2024 •

edited

Loading

hgraphql commented Apr 23, 2024 •

edited

Loading