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

Feature/wallet connect 2.0 #236

Merged
merged 3 commits into from
Oct 3, 2023
Merged

Feature/wallet connect 2.0 #236

merged 3 commits into from
Oct 3, 2023

Conversation

poocart
Copy link
Contributor

@poocart poocart commented Aug 23, 2023

Description

Motivation and Context

  • Per description.

How Has This Been Tested?

  • Locally with React dapp and wagmi connector.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@@ -196,6 +203,10 @@ export class Sdk {
constructor(walletProvider: WalletProviderLike, optionsLike?: EnvNames | SdkOptions) {
let options: SdkOptions = {};

if (isWalletConnectProvider(walletProvider)) {
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 lets to avoid any additional mappings before passing Wallet Connect provider to Etherspot SDK constructor

@@ -17,7 +17,8 @@
"ES5",
"es2015.collection",
"es2015.iterable"
]
],
"skipLibCheck": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

skips types check on node_modules

Copy link

@IAmKio IAmKio left a comment

Choose a reason for hiding this comment

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

Just some minor questions but other than that - looks good!

Comment on lines +10 to +20
try {
const {
accounts: [address],
chainId,
} = provider;

this.setAddress(address);
this.setNetworkName(chainId);
} catch (err) {
//
}
Copy link

Choose a reason for hiding this comment

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

Just out of curiosity is there a reason we're using try / catch here as if accounts / chainId are not found in provider it would just return undefined for the destrucutred vartiables?

Copy link
Contributor Author

@poocart poocart Aug 24, 2023

Choose a reason for hiding this comment

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

this is copy paste implementation of previous Wallet Connect provider

Comment on lines +33 to +36
const response = await this.provider.signer.request({
method: 'personal_sign',
params: [toHex(message), this.address],
});
Copy link

Choose a reason for hiding this comment

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

Just wondering if htis could return an error from the await?

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 is copy paste implementation of previous Wallet Connect provider, it can return error, but it wasn't handled in previous Wallet Connect provider so didn't feel to add anything extra that is beyond previous structure

({
accounts: [address],
chainId,
} = payload.params[0]);
Copy link

Choose a reason for hiding this comment

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

This is definitely valid due to the [0]

@poocart poocart requested a review from IAmKio August 24, 2023 10:34
@ch4r10t33r
Copy link
Member

@vignesha22 @marie-fourier Pls review this

Copy link
Member

@ch4r10t33r ch4r10t33r left a comment

Choose a reason for hiding this comment

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

LGTM

@poocart poocart merged commit e324d3c into master Oct 3, 2023
5 checks passed
@poocart poocart deleted the feature/wallet-connect-2.0 branch October 3, 2023 12:54
@poocart poocart restored the feature/wallet-connect-2.0 branch October 3, 2023 12:55
@poocart poocart deleted the feature/wallet-connect-2.0 branch October 3, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants