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

[Push] Add wc_pushPropose method #872

Merged
merged 21 commits into from
May 30, 2023
Merged

[Push] Add wc_pushPropose method #872

merged 21 commits into from
May 30, 2023

Conversation

llbartekll
Copy link
Contributor

Description

  • Add wc_pushPropose method
  • Remove wc_pushRequest

Resolves # (issue)

How Has This Been Tested?

Due Dilligence

  • Breaking change
  • Requires a documentation update

@arein arein added the accepted label May 26, 2023
@llbartekll llbartekll marked this pull request as draft May 26, 2023 06:55
@llbartekll llbartekll temporarily deployed to internal May 29, 2023 09:26 — with GitHub Actions Inactive
@llbartekll llbartekll temporarily deployed to internal May 29, 2023 09:39 — with GitHub Actions Inactive
@llbartekll llbartekll temporarily deployed to internal May 29, 2023 09:42 — with GitHub Actions Inactive
@llbartekll llbartekll temporarily deployed to internal May 29, 2023 12:07 — with GitHub Actions Inactive
@llbartekll llbartekll marked this pull request as ready for review May 29, 2023 12:23
@llbartekll llbartekll requested a review from flypaper0 May 29, 2023 12:23
let metadata = AppMetadata(name: "GM Dapp", description: "", url: "https://gm-dapp-xi.vercel.app/", icons: [])
try! await walletPushClient.subscribe(metadata: metadata, account: Account.stub(), onSign: sign)
walletPushClient.subscriptionsPublisher
.first()
Copy link
Contributor

Choose a reason for hiding this comment

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

why first here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because after calling deleteSubscription, subscriptionsPublisher will publish again with an empty array

@@ -30,8 +30,8 @@ final class MainRouter {
}

func present(pushRequest: PushRequest) {
PushRequestModule.create(app: app, pushRequest: pushRequest)
.presentFullScreen(from: viewController, transparentBackground: true)
// PushRequestModule.create(app: app, pushRequest: pushRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

method unused now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just want to showcase w3i capabilities and limitations for dogfooding. This presents a native screen.

let protocolMethod = NotifyProposeProtocolMethod()
networkingInteractor.responseErrorSubscription(on: protocolMethod)
.sink { [unowned self] (payload: ResponseSubscriptionErrorPayload<NotifyProposeParams>) in
kms.deletePrivateKey(for: payload.request.publicKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

As we're removing private key, should we also unsubscribe on error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch,
btw, dapp is not supported in this version, it's mainly developed and maintained for integration testing

let subscriptionAuthWrapper = try await pushSubscribeRequester.subscribe(metadata: proposal.metadata, account: proposal.account, onSign: onSign)

var pushSubscription: PushSubscription!
try await withCheckedThrowingContinuation { continuation in
Copy link
Contributor

Choose a reason for hiding this comment

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

As we do not have subscription in this point, may be we should listen subscriptionResponsePublisher separately? Not inside approve method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 hmm, like how? I kind of want to block a thread here and wait for only the first subscription

@@ -91,55 +91,40 @@ final class PushTests: XCTestCase {
makeWalletClients()
}

func testRequestPush() async {
Copy link
Contributor

Choose a reason for hiding this comment

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

push request step removed from protocol right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly, the method is deprecated

@llbartekll llbartekll temporarily deployed to internal May 29, 2023 17:04 — with GitHub Actions Inactive
@llbartekll llbartekll changed the title Add wc_pushPropose method [Push] Add wc_pushPropose method May 30, 2023
@llbartekll llbartekll merged commit 60bcb15 into develop May 30, 2023
@llbartekll llbartekll deleted the push-propose branch May 30, 2023 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants