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

#376 auth request service #385

Merged
merged 7 commits into from
Aug 2, 2022
Merged

#376 auth request service #385

merged 7 commits into from
Aug 2, 2022

Conversation

llbartekll
Copy link
Contributor

@llbartekll llbartekll commented Jul 29, 2022

  • implements authRequestService
  • adds auth granular tag
  • implements networking interactor's request method

@llbartekll llbartekll requested a review from flypaper0 July 29, 2022 07:58
@llbartekll llbartekll self-assigned this Jul 29, 2022
@llbartekll llbartekll changed the base branch from main to develop July 29, 2022 07:58
@llbartekll llbartekll marked this pull request as draft July 29, 2022 10:51
@llbartekll llbartekll marked this pull request as ready for review August 1, 2022 08:48
@llbartekll llbartekll requested a review from sekimondre August 1, 2022 08:49
}
}

// TODO - temporarly duplicated - moved do utils in concurrent PR

Choose a reason for hiding this comment

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

I believe that, if this is moved to Utils, we'll have to expose it via a typealias in each SDK. Remember to test it if you do

let issueAt = ISO8601DateFormatter().string(from: Date())
let payload = AuthPayload(requestParams: params, iat: issueAt)
let params = AuthRequestParams(requester: requester, payloadParams: payload)
let request = JSONRPCRequest<AuthRequestParams>(method: "wc_authRequest", params: params)

Choose a reason for hiding this comment

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

What do you think of using RPCRequest in here? Seems like a good place to use it

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 was waiting for your first demo integration 😄 but ok, let's give it a shot.

class NetworkingInteractor: NetworkInteracting {
private let relayClient: RelayClient
private let serializer: Serializing
private let jsonRpcHistory: JsonRpcHistory<AuthRequestParams>

Choose a reason for hiding this comment

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

As it was already merged to develop, what do you also think of using the improved RPCHistory in here, so you can avoid binding the storage to a single specialized type?

@sekimondre
Copy link

Left some suggestions to incentivise scaling the service from the start. Also, the PR needs to run again in the CI (maybe rebase with develop)

@llbartekll llbartekll merged commit db0f22f into develop Aug 2, 2022
@llbartekll llbartekll deleted the #376-auth-request-service branch August 2, 2022 07:48
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