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

Web3Wallet implementation + integration tests #638

Merged
merged 5 commits into from
Jan 1, 2023
Merged

Conversation

alexander-lsvk
Copy link
Contributor

@alexander-lsvk alexander-lsvk commented Dec 17, 2022

Description

Web3Wallet proxy implementation according to the interface described here

  • Breaking change
  • Requires a documentation update

signerFactory: config.signerFactory,
networkingClient: Networking.interactor,
pairingRegisterer: Pair.registerer,
pairingClient: Pair.instance as! PairingClient
Copy link
Contributor

Choose a reason for hiding this comment

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

@llbartekll as I remember we found the way how to avoid force casts for pairing client. Or we decided to leave it like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have fixed it for Networking having two vars: instance and interactor I think we could do the same with pairing

@@ -0,0 +1,3 @@
#if !CocoaPods
@_exported import WalletConnectPairing
Copy link
Contributor

Choose a reason for hiding this comment

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

also need to remove all packages imports from all files

Copy link
Contributor

Choose a reason for hiding this comment

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

Cocoapods putting all sources in single framework, so it doesn't know what is Auth, WalletConnectSign etc

Need to remove all

import WalletConnectSign
import Auth

And imports should be

@_exported import Auth
@_exported import WalletConnectSign

Agree it looks like a little wired but supporting both SPM and Cocoapods is a kind of pain

@alexander-lsvk
Copy link
Contributor Author

alexander-lsvk commented Dec 21, 2022

@llbartekll @flypaper0 I added unit tests for Web3Wallet in the last commit. It required adding abstraction levels for Auth, Sign and Pair -> AuthClientProtocol, SignClientProtocol, PairClientProtocol.

I'm not really satisfied with this *Protocol suffix, I'd be more pleased to have protocol named, i.e. SignClient, and then SignClientMock and SignClient*Something* for implementation. But the question is what this Something should be 🤔

Any suggestions?

Copy link
Contributor

@llbartekll llbartekll 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.

@flypaper0
Copy link
Contributor

@llbartekll @flypaper0 I added unit tests for Web3Wallet in the last commit. It required adding abstraction levels for Auth, Sign and Pair -> AuthClientProtocol, SignClientProtocol, PairClientProtocol.

I'm not really satisfied with this *Protocol suffix, I'd be more pleased to have protocol named, i.e. SignClient, and then SignClientMock and SignClient*Something* for implementation. But the question is what this Something should be 🤔

Any suggestions?

For me all namings is good, but it must be consistant across project and all our sdk's

Copy link
Contributor

@flypaper0 flypaper0 left a comment

Choose a reason for hiding this comment

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

lgtm!

# Conflicts:
#	Example/ExampleApp.xcodeproj/project.pbxproj
@alexander-lsvk alexander-lsvk merged commit 3a6c77a into develop Jan 1, 2023
@alexander-lsvk alexander-lsvk deleted the wallet-sdk branch January 1, 2023 22:43
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.

3 participants