-
Notifications
You must be signed in to change notification settings - Fork 8
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 WalletConnect signing manager #50
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lgtm. I left some nitty comment + there are sonar cloud warnings that should be taken care off.
packages/walletconnect-signing-manager/src/lib/walletconnect-signing-manager.spec.ts
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect-signing-manager.spec.ts
Outdated
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect-signing-manager.spec.ts
Outdated
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect-signing-manager.spec.ts
Outdated
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect-signing-manager.spec.ts
Outdated
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect-signing-manager.ts
Outdated
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect-signing-manager.ts
Outdated
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect/signer.spec.ts
Outdated
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect/walletconnect.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you are actually deregistering the listeners on disconnect based on JS always comparing with referential equality. I missed that on the first review.
Also some nits around as any
warnings.
Other than that LGTM
packages/walletconnect-signing-manager/src/lib/walletconnect/walletconnect.spec.ts
Outdated
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect/walletconnect.spec.ts
Outdated
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect/walletconnect.ts
Outdated
Show resolved
Hide resolved
packages/walletconnect-signing-manager/src/lib/walletconnect/walletconnect.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. I left one optional comment
packages/walletconnect-signing-manager/src/lib/walletconnect/walletconnect.ts
Outdated
Show resolved
Hide resolved
|
🎉 This PR is included in version @polymeshassociation/[email protected] 🎉 The release is available on: |
🎉 This PR is included in version 3.4.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Adds a new signing manager that allows remote wallets to connect to the Polymesh SDK using WalletConnect.
Breaking Changes
NA
JIRA Link
NA
Checklist