v1.3.6 Improved Session Handling and Logging #318

kantorcodes · 2024-10-29T20:33:45Z

Description:

This PR suggests a number of fixes to improve the dApp experience:

Enables dApps to specify a log level for better control of console logs.
Ensures that there is only one Signer per AccountId + ExtensionId combination. Having multiple signers with different ids created issues in pulling the correct record.
Explores better state management and automatic removal of stale sessions.

Related issue(s):

Fixes CON-494

Notes for reviewer:

We need to merge v1.3.5 Improved Documentation and Test Runner #317 first and then update this branch before merging this in.
These updates are substantial and should be a net positive on the experience, but require careful review from the dApp experience. Wallet experiences should remain mostly the same.

Checklist

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

Signed-off-by: Michael Kantor <[email protected]>

franfernandez20 · 2024-11-05T10:37:03Z

src/lib/dapp/index.ts

+      const signClient = await SignClient.init({
        logger,
        relayUrl: 'wss://relay.walletconnect.com',
        projectId: this.projectId,
        metadata: this.dAppMetadata,
      })
+      this.walletConnectClient = signClient
      const existingSessions = this.walletConnectClient.session.getAll()

-      if (existingSessions.length > 0)
+      if (existingSessions.length > 0) {
        this.signers = existingSessions.flatMap((session) => this.createSigners(session))
-      else this.checkIframeConnect()
+      } else {
+        await this.checkIframeConnect()
+      }


I agree with improving code style and formatting, but these changes should be in a separate commit to avoid any confusion with the purpose of this one.

franfernandez20 · 2024-11-05T10:40:29Z

src/lib/shared/logger.ts

+export interface ILogger {
+  error(message: string, ...args: any[]): void
+  warn(message: string, ...args: any[]): void
+  info(message: string, ...args: any[]): void
+  debug(message: string, ...args: any[]): void
+}
+
+export class DefaultLogger implements ILogger {
+  private logLevel: 'error' | 'warn' | 'info' | 'debug' = 'info'
+
+  constructor(logLevel: 'error' | 'warn' | 'info' | 'debug' = 'info') {
+    this.logLevel = logLevel
+  }
+
+  setLogLevel(level: 'error' | 'warn' | 'info' | 'debug'): void {
+    this.logLevel = level
+  }
+
+  error(message: string, ...args: any[]): void {
+    if (['error', 'warn', 'info', 'debug'].includes(this.logLevel)) {
+      console.error(`[ERROR] ${message}`, ...args)
+    }
+  }
+
+  warn(message: string, ...args: any[]): void {
+    if (['warn', 'info', 'debug'].includes(this.logLevel)) {
+      console.warn(`[WARN] ${message}`, ...args)
+    }
+  }
+
+  info(message: string, ...args: any[]): void {
+    if (['info', 'debug'].includes(this.logLevel)) {
+      console.info(`[INFO] ${message}`, ...args)
+    }
+  }
+
+  debug(message: string, ...args: any[]): void {
+    if (this.logLevel === 'debug') {
+      console.debug(`[DEBUG] ${message}`, ...args)
+    }
+  }
+}


This is a great idea! However, I'd suggest keeping the logging feature separate from the wallet connect functionality. Putting it in its own commit will keep things clear.

franfernandez20 · 2024-11-05T10:42:00Z

src/lib/shared/utils.ts

+  return transaction._makeTransactionBody(nodeAccountId)
 }

-export function transactionBodyToBase64String(transactionBody: Uint8Array) {
-  return Uint8ArrayToBase64String(transactionBody)
+export function transactionBodyToBase64String(transactionBody: proto.ITransactionBody) {
+  return Uint8ArrayToBase64String(proto.TransactionBody.encode(transactionBody).finish())
 }


Same here, this fixes a reported issue, so it should be committed separately and liked to it.

franfernandez20 · 2024-11-05T10:58:50Z

Great work, @kantorcodes ! This is a fantastic improvement. The only thing I’d suggest is splitting these changes into four parts:

Logging System Implementation
Session Update Enhancements
Issue with transactionBodyToBase64String Resolution. (PR125)
Code Style Improvements

kantorcodes · 2024-11-05T11:26:13Z

Thank you for your feedback.

While I generally agree that breaking this out into multiple commits would make this easier in case we must revert something, at the same time the work here provides essential fixes to make HWC more usable for important dApps in the ecosystem.

Perhaps prioritizing canary releases would be a good interim solution, as I fear the time to review four separate pull requests would slow things down immensely.

Additionally, while logging is not a required dependency here, it did make developing and debugging these improvements easier.

My proposed options in order:

A) prioritize canary NPM tags so we can allow immediate usage of these improvements to dApps

B) Push this one over in light of urgency

C) Counter recommendations for two PRs instead of four

remove style improvements
combine session improvements + logging
issue a revert PR on transaction bytes separately

itsbrandondev · 2024-11-05T14:33:00Z

@kantorcodes i really like the idea of utilizing canary releases (option A). we have to find a balance between urgent response for dApps and plenty of time for reviews and testing. option C is reasonable as well.

interested to hear thoughts from @teacoat @valeraOlexienko @jwtong

Signed-off-by: Michael Kantor <[email protected]>

codecov · 2024-11-05T17:07:51Z

Codecov Report

Attention: Patch coverage is 6.97674% with 40 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@b3600fd). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/lib/dapp/index.ts	8.33%	33 Missing ⚠️
src/lib/dapp/DAppSigner.ts	0.00%	4 Missing ⚠️
src/lib/shared/utils.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #318   +/-   ##
=======================================
  Coverage        ?   36.05%           
=======================================
  Files           ?       14           
  Lines           ?      613           
  Branches        ?       79           
=======================================
  Hits            ?      221           
  Misses          ?      392           
  Partials        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Michael Kantor <[email protected]>

kantorcodes · 2024-11-05T17:29:20Z

@kantorcodes i really like the idea of utilizing canary releases (option A). we have to find a balance between urgent response for dApps and plenty of time for reviews and testing. option C is reasonable as well.

interested to hear thoughts from @teacoat @valeraOlexienko @jwtong

After some discovery, we've figured out we can create canary releases now. Per the feedback, I'll be breaking off this PR into separate merges.

Signed-off-by: Michael Kantor <[email protected]>

kantorcodes · 2024-11-05T21:18:04Z

We've broken this PR into three new reviews, and will be closing this one out in favor of those.

#327
#326
#325

andrewb1269hg

@rbarkerSL I think these 3 files look good, but please review as well:
.github/workflows/publish-canary.yml - new file, definitely review.
package-lock.json (doesn't really need to be reviewed)
package.json

kantorcodes requested review from a team as code owners October 29, 2024 20:33

kantorcodes requested review from andrewb1269hg, nathanklick and web3-nomad October 29, 2024 20:33

kantorcodes force-pushed the CON-494 branch 7 times, most recently from aed5926 to fe12330 Compare October 31, 2024 19:27

feat: improved logging + handling

b7262e7

Signed-off-by: Michael Kantor <[email protected]>

kantorcodes force-pushed the CON-494 branch from fe12330 to b7262e7 Compare October 31, 2024 19:32

kantorcodes added 6 commits October 31, 2024 15:34

fix: package spacing

ee9aadf

Signed-off-by: Michael Kantor <[email protected]>

ci: update test pipeline for canary

3178a19

Signed-off-by: Michael Kantor <[email protected]>

ci: update test pipeline for canary

d247b27

Signed-off-by: Michael Kantor <[email protected]>

ci: update test pipeline for canary

88e539d

Signed-off-by: Michael Kantor <[email protected]>

ci: update pipeline

e834438

Signed-off-by: Michael Kantor <[email protected]>

ci: try seperate workflow

56cc93f

Signed-off-by: Michael Kantor <[email protected]>

kantorcodes force-pushed the CON-494 branch from 88dc36e to 4b4e03f Compare November 4, 2024 15:56

kantorcodes added 4 commits November 4, 2024 10:56

fix: retrieve actual signer

4738402

Signed-off-by: Michael Kantor <[email protected]>

fix: add error handling for wrong format

f712105

Signed-off-by: Michael Kantor <[email protected]>

fix: check name for metadata check

c5541ce

Signed-off-by: Michael Kantor <[email protected]>

fix: revert transactionbody code

5ba384b

Signed-off-by: Michael Kantor <[email protected]>

kantorcodes force-pushed the CON-494 branch from 4b4e03f to 5ba384b Compare November 4, 2024 15:56

ci: comment out canary pipeline

2e3e69e

Signed-off-by: Michael Kantor <[email protected]>

Merge branch 'main' into CON-494

fbf5b55

jwtong approved these changes Nov 4, 2024

View reviewed changes

franfernandez20 reviewed Nov 5, 2024

View reviewed changes

kantorcodes added 5 commits November 5, 2024 09:52

ci: use prepatch

6d639a7

Signed-off-by: Michael Kantor <[email protected]>

ci: try pull_request_target

a698775

Signed-off-by: Michael Kantor <[email protected]>

ci: rm canary workflow

8088280

Signed-off-by: Michael Kantor <[email protected]>

ci: restore test.yml

4e4ee51

Signed-off-by: Michael Kantor <[email protected]>

ci: readd canary release

aa447da

Signed-off-by: Michael Kantor <[email protected]>

kantorcodes had a problem deploying to github-actions November 5, 2024 16:55 — with GitHub Actions Failure

kantorcodes had a problem deploying to Actions November 5, 2024 16:56 — with GitHub Actions Failure

kantorcodes force-pushed the CON-494 branch from 9727daf to aa447da Compare November 5, 2024 17:04

ci: no git tag on prerelease

898121c

Signed-off-by: Michael Kantor <[email protected]>

kantorcodes force-pushed the CON-494 branch from 9b42a86 to 898121c Compare November 5, 2024 17:28

kantorcodes mentioned this pull request Nov 5, 2024

fix: improved session handling and logging #327

Merged

2 tasks

fix: remove async

9987749

Signed-off-by: Michael Kantor <[email protected]>

andrewb1269hg reviewed Nov 6, 2024

View reviewed changes

kantorcodes closed this Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1.3.6 Improved Session Handling and Logging #318

v1.3.6 Improved Session Handling and Logging #318

kantorcodes commented Oct 29, 2024 •

edited

Loading

franfernandez20 Nov 5, 2024

franfernandez20 Nov 5, 2024

franfernandez20 Nov 5, 2024

franfernandez20 commented Nov 5, 2024 •

edited

Loading

kantorcodes commented Nov 5, 2024 •

edited

Loading

itsbrandondev commented Nov 5, 2024

codecov bot commented Nov 5, 2024

kantorcodes commented Nov 5, 2024

kantorcodes commented Nov 5, 2024 •

edited

Loading

andrewb1269hg left a comment

v1.3.6 Improved Session Handling and Logging #318

v1.3.6 Improved Session Handling and Logging #318

Conversation

kantorcodes commented Oct 29, 2024 • edited Loading

franfernandez20 Nov 5, 2024

Choose a reason for hiding this comment

franfernandez20 Nov 5, 2024

Choose a reason for hiding this comment

franfernandez20 Nov 5, 2024

Choose a reason for hiding this comment

franfernandez20 commented Nov 5, 2024 • edited Loading

kantorcodes commented Nov 5, 2024 • edited Loading

itsbrandondev commented Nov 5, 2024

codecov bot commented Nov 5, 2024

Codecov Report

kantorcodes commented Nov 5, 2024

kantorcodes commented Nov 5, 2024 • edited Loading

andrewb1269hg left a comment

Choose a reason for hiding this comment

kantorcodes commented Oct 29, 2024 •

edited

Loading

franfernandez20 commented Nov 5, 2024 •

edited

Loading

kantorcodes commented Nov 5, 2024 •

edited

Loading

kantorcodes commented Nov 5, 2024 •

edited

Loading