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

Add support for multiple sessions #135

Merged
merged 2 commits into from
May 22, 2024

Conversation

franfernandez20
Copy link
Contributor

This commit introduces support for multiple sessions via Wallet Connect client, enabling developers to manage various accountIds simultaneously.

Redundant functions have been removed for cleaner code.

Notes for reviewer:
This pull request is part of PR#76, which will be permanently deleted once all topics are split.

Checklist

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

This commit introduces support for multiple sessions via Wallet Connect
client, enabling developers to manage various accountIds simultaneously.

Redundant functions have been removed for cleaner code.

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

@hgraphql hgraphql left a comment

Choose a reason for hiding this comment

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

looks good to me!

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

private async checkPersistedState() {
Copy link
Contributor

Choose a reason for hiding this comment

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

My review doesn't account for removing checkPersistedState or pingWithRetry. I don't think I'm qualified to review if these can be removed :)

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

if (existingSessions.length) {
await this.onSessionConnected(existingSessions.pop()!)
const existingSessions = this.walletConnectClient.session.getAll()
Copy link

Choose a reason for hiding this comment

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

Could you please explain a bit why the logic for checking really alive sessions has been removed and simplified to this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was designed to remove sessions when the corresponding wallet in the communication fails to respond. Non-responsiveness can occur if the wallet is closed or temporarily inaccessible; however, it is crucial that the session remains valid during such intervals. The getAll method from WalletConnect core is tasked with reinstating any session that should remain active, ensuring continuity and stability in the communication process.

Signed-off-by: Fran Fernandez <fran@kabila.app>
@hgraphql hgraphql requested review from a team, matteriben and jeromy-cannon May 22, 2024 18:19
Copy link
Contributor

@gregscullard gregscullard left a comment

Choose a reason for hiding this comment

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

LGTM

@hgraphql
Copy link
Contributor

Nice work!

@hgraphql hgraphql merged commit d6ff57b into hashgraph:main May 22, 2024
7 checks passed
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.

5 participants