Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Mobile client #471

Merged
merged 29 commits into from Jul 30, 2020
Merged

Mobile client #471

merged 29 commits into from Jul 30, 2020

Conversation

Robert-Steiner
Copy link
Contributor

@Robert-Steiner Robert-Steiner commented Jul 20, 2020

Not done yet but I can already be reviewed.

What is done:

I actually wanted to create a PR for each module Participant, ClientState and MobileClient. However, when I implemented the ClientState struct, I had to change a lot on the participant struct and it got a bit messy. For this reason, I packed everything in one PR. If you want I can also create a new PR for each module.

My suggestion would be that we keep the old client/participant for now and just add the mobile client to the repository and first test if the mobile approach works

@little-dude little-dude self-requested a review July 21, 2020 07:30

pub struct ParticipantState {
// credentials
pub pk: ParticipantPublicKey,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could use the EncryptKeyPair for this now.

Copy link
Contributor

Choose a reason for hiding this comment

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

SigningKeyPair

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah true, I will change 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.

Shall we do the same with

ephm_pk: SumParticipantEphemeralPublicKey,
ephm_sk: SumParticipantEphemeralSecretKey,

in Participant<Sum> and Participant<Sum2> as well?

rust/src/client/mobile_client/participant/mod.rs Outdated Show resolved Hide resolved
rust/src/client/mobile_client/client.rs Show resolved Hide resolved
Copy link
Contributor

@janpetschexain janpetschexain 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! i'm happy to help with the ffi parts.

rust/src/client/mobile_client/mod.rs Outdated Show resolved Hide resolved
rust/src/client/mobile_client/client.rs Outdated Show resolved Hide resolved
rust/src/client/mobile_client/client.rs Outdated Show resolved Hide resolved

pub struct ParticipantState {
// credentials
pub pk: ParticipantPublicKey,
Copy link
Contributor

Choose a reason for hiding this comment

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

SigningKeyPair

rust/src/bin/test-mobile.rs Outdated Show resolved Hide resolved
@Robert-Steiner Robert-Steiner force-pushed the mobile-client branch 5 times, most recently from 35726d4 to 124ba8d Compare July 24, 2020 19:30
@Robert-Steiner
Copy link
Contributor Author

Robert-Steiner commented Jul 29, 2020

@finiteprods @little-dude @janpetschexain
ready for review🙂

changes since the last review

  • implemented the function new_secret_key to make it easier for a user to generate a new secret key
  • implemented the FFI (currently, it is living in a separate repository)
  • made the rustls feature of reqwest configurable on the xaynet crate (we had issues compiling reqwest on android due to openssl therefore we switched to rustls)
  • moved the mobile client example into the example folder

Note:
The new participant/client struct and the new mobile client are still in an early stage. For that reason, the documentation of the mobile client is hidden. However, the new participant/client struct is a starting point for a unified client state machine that works on desktop, web(wasm) and mobile.

rust/src/client/mobile_client/client.rs Outdated Show resolved Hide resolved
rust/src/client/mobile_client/client.rs Outdated Show resolved Hide resolved
@little-dude little-dude self-requested a review July 29, 2020 14:07
rust/src/client/mobile_client/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@little-dude little-dude left a comment

Choose a reason for hiding this comment

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

This is great 👍. We should clean the commit history up before merging. I don't know if you prefer to address https://github.com/xaynetwork/xaynet/pull/471/files#r462925062 now or later in a follow-up PR. I'm fine with both.

@Robert-Steiner
Copy link
Contributor Author

I removed the Option, implemented impl Into<Participant<Sum2>> for Participant<Sum> and renamed the method next of the mobile client to perform_task

@little-dude
I agree, it's okay if I spuash the PR?

@little-dude
Copy link
Contributor

Nice, I think this is an improvement 👍

@little-dude I agree, it's okay if I squash the PR?

Sure or just splitting it in a bunch of self-contained commits works as well 👍

@little-dude little-dude self-requested a review July 30, 2020 14:20
@Robert-Steiner Robert-Steiner merged commit 942c926 into master Jul 30, 2020
@Robert-Steiner Robert-Steiner deleted the mobile-client branch July 30, 2020 15:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants