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

fix: improved session handling and logging #327

Merged
merged 11 commits into from
Nov 13, 2024
Merged

fix: improved session handling and logging #327

merged 11 commits into from
Nov 13, 2024

Conversation

kantorcodes
Copy link
Contributor

@kantorcodes kantorcodes commented Nov 5, 2024

Description:
Broken off from #318

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

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

Improves

Related issue(s):

#254

Fixes #

Notes for reviewer:

This PR can be tested individually by installing the canary release:

Checklist

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

@kantorcodes kantorcodes requested review from a team as code owners November 5, 2024 20:01
Copy link

github-actions bot commented Nov 5, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
76.29% (+37.69% 🔼)
444/582
🟡 Branches
63.48% (+35.18% 🔼)
73/115
🟡 Functions
75.69% (+50.13% 🔼)
109/144
🟡 Lines
77.28% (+37.59% 🔼)
415/537
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢 index.ts 100% 100% 100% 100%
🟢 lib/index.ts 100% 100% 100% 100%
🟢 lib/shared/index.ts 100% 100% 100% 100%
🟢
... / chainIds.ts
100% 100% 100% 100%
🔴
... / errors.ts
20% 0% 0% 25%
🟢
... / events.ts
100% 100% 100% 100%
🟢
... / methods.ts
100% 100% 100% 100%
🟢 lib/shared/utils.ts 86.96% 66.67% 86.21% 88.46%
🔴
... / extensionController.ts
30.77% 11.11% 16.67% 28.57%
🔴 lib/wallet/index.ts 37.74% 28.57% 50% 39%
🔴
... / provider.ts
38.46% 100% 27.27% 38.46%
🟢 lib/dapp/index.ts 86.83% 77.5% 83.33% 87.83%
🟢
... / logger.ts
100% 100% 100% 100%
🟢
... / DAppSigner.ts
100% 86.96% 100% 100%

Test suite run success

110 tests passing in 11 suites.

Report generated by 🧪jest coverage report action from d723d56

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 8.00000% with 46 lines in your changes missing coverage. Please review.

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

Files with missing lines Patch % Lines
src/lib/dapp/index.ts 8.16% 45 Missing ⚠️
src/lib/dapp/DAppSigner.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #327   +/-   ##
=======================================
  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]>
@franfernandez20
Copy link
Contributor

I don’t see what has been split here, it still looks like code styling changes along with logging and some session management updates. The updates look solid, however, I still recommend splitting this PR into smaller, more focused ones for clarity. That said, I’m open to moving forward if others feel differently—I don’t want to hold up progress.

@tmctl
Copy link
Contributor

tmctl commented Nov 11, 2024

I don’t see what has been split here, it still looks like code styling changes along with logging and some session management updates. The updates look solid, however, I still recommend splitting this PR into smaller, more focused ones for clarity. That said, I’m open to moving forward if others feel differently—I don’t want to hold up progress.

I think a net of 170 lines of code and 6 files changed is an okay sized PR. Here's an example PR into the javascript sdk that is similarily sized - https://github.com/hashgraph/hedera-sdk-js/pull/2631/files

I'd say we can probably move forward with this size PR

@justynspooner
Copy link
Contributor

The code updates look good to me. It's hard to be confident with the updates though without test coverage especially considering it's mitigating against multiple connection states which would be hard to reproduce. Are there some tests we can add to help raise confidence with these fixes?

@kantorcodes kantorcodes requested a review from a team as a code owner November 12, 2024 17:25
@kantorcodes
Copy link
Contributor Author

The code updates look good to me. It's hard to be confident with the updates though without test coverage especially considering it's mitigating against multiple connection states which would be hard to reproduce. Are there some tests we can add to help raise confidence with these fixes?

Great flag. Just pushed a heap of tests to the PR. Could you check again and let me know your thoughts? We'll also have coverage on the connector which was previously not running.

console.log(`Dapp: Pairing deleted by wallet!`)
// clean up after the pairing for `topic` was deleted.
})
this.walletConnectClient.on('session_event', this.handleSessionEvent.bind(this))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, these callbacks were refactored in order to make it easier to unit test

Signed-off-by: Michael Kantor <[email protected]>
@jwtong
Copy link

jwtong commented Nov 13, 2024

Looks good to me. Is it possible to move this past the finish line quickly and merge it? It’ll be a major help for us at Saucerswap to address WC reliability on our platform. Thanks!

Signed-off-by: Michael Kantor <[email protected]>
Signed-off-by: Michael Kantor <[email protected]>
@kantorcodes kantorcodes merged commit cb75b86 into main Nov 13, 2024
17 checks passed
@kantorcodes kantorcodes deleted the fix/CON-494 branch November 13, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants