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

Implement WebSocket transport #934

Merged
merged 3 commits into from
May 6, 2021
Merged

Implement WebSocket transport #934

merged 3 commits into from
May 6, 2021

Conversation

sergeyboyko0791
Copy link

  • Add WebSocket transport working in Wasm
  • Add StateMachine pattern

Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Great, I like the state machine implementation 🙂 It's fine to merge this now. As discussed in person, the further plan is:

  1. Refactor implementation so both outgoing_tx and incoming_rx should be dropped for connection to close, not only outgoing_tx.
  2. Reduce the boilerplate if possible.
  3. Integrate the transport into Electrum client.

And I have 1 more question 🙂

/* vvv The access guards that prevents the user using this pattern from entering an invalid state vvv */

/// An instance of `ChangeGuard` can be initialized within `state_machine` module only.
pub struct ChangeGuard<Ctx, Result> {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please provide more details on the ChangeGuard? It's unclear for me now how it prevents "from entering an invalid state" 🙂

Copy link
Author

Choose a reason for hiding this comment

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

ChangeGuard has to be initialized within the StateExt::change_state method only because this StateExt::change_state method performs the compile-time validation of whether Self (for example, ConnectingState) can transition to the Next state (for example, OpenState).
If a user could initialize ChangeGuard outside the state_machine.rs file, he might enter an invalid state (for example, ClosingState -> OpenState). So it's the reason why ChangeGuard or ResultGuard can be initialized within the state_machine.rs file only (they have private fields and their constructors are private).

Copy link
Author

Choose a reason for hiding this comment

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

I forgot to mention that if you want to change the state, you have to return StateResult::ChangeState(ChangeGuard). But you cannot construct ChangeGuard manually, so you will do something like return Self::change_state where Self::change_state performs the transition validation in compile-time and returns StateResult::ChangeState(ChangeGuard).

Copy link
Author

@sergeyboyko0791 sergeyboyko0791 May 6, 2021

Choose a reason for hiding this comment

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

As you noticed yesterday, a user implements State::on_changed for his custom state. This method should return StateResult::ChangeState(ChangeGuard) to change the state to the next one. So the user has to use Self::change_state that protects him from entering an invalid state

@artemii235 artemii235 requested a review from shamardy May 5, 2021 12:22
@artemii235
Copy link
Member

@shamardy Please check this too, maybe you will have some questions 🙂 We will possibly use this state machine pattern in other places soon.

type Result = T::Result;

/// The last state always returns the result of the state machine calculations.
async fn on_changed(self: Box<Self>, ctx: &mut T::Ctx) -> StateResult<Self::Ctx, Self::Result> {
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if we can avoid boxing here and in other places if present? It will be awesome if ever possible 🙂

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, I tried to avoid boxing but didn't find a way to do it. I'm not sure if I can explain it in a simple and understandable way :D
StateResult cannot depend on a Next type, because State::on_changed may return various next states (therefore various Next types), but Rust doesn't allow to do this. It's because StateResult contains Box<dyn State>.

Secondly, let's imagine that we have a trait like this:

trait State {
    fn on_changed(&self) -> StateResult;
}
let obj: Box<dyn State> = todo!();
// We can call the `State::on_changed` method on the `Box<dyn State>` object.
obj.on_changed();

Lets change the trait:

trait State {
    fn on_changed(self) -> StateResult;
    fn on_changed_boxed(self: Box<Self>) -> StateResult;
}
let obj: Box<dyn State> = todo!();
// We cannot call the `State::on_changed` method on the `Box<dyn State>` object.
// obj.on_changed();

// Only if it takes a boxed self.
obj.on_changed_boxed();

Copy link
Collaborator

@shamardy shamardy 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 work @sergeyboyko0791, as far as I can understand 😄

@sergeyboyko0791
Copy link
Author

This is great work @sergeyboyko0791, as far as I can understand smile

Thanks a lot! Hopefully, this implementation doesn't have disadvantages
But as you can see, this pattern is not very suitable for WebSocket transport implementation 😄
There is a lot of boilerplate code that I'm planning to remove.

@artemii235 artemii235 merged commit 7b0f768 into dev May 6, 2021
@artemii235 artemii235 deleted the mm2.1-wasm-electrum branch May 6, 2021 08:09
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