search

magic-wormhole / magic-wormhole.rs

Rust implementation of Magic Wormhole, with new features and enhancements
European Union Public License 1.2
752 stars 80 forks source link

Input machine should not panic on invalid state transfer, especially in case of wormhole send. #44

Closed copyninja closed 3 years ago

copyninja commented 6 years ago

Hi,

While reviewing send example code under io/blocking and also core's example ws.rs I noticed that recent changes to input.rs causes it to panic. This happens because InputEvent::GotWordlist is generated before Input machine is started. In case of sending it is normal that input machine is not started, since user is not manually inputting the code using tab completion. 2 possibility of setting code is either via SetCode or AllocateCode both of which does not need Input machine.

Previously code was working fine because I was not generating panic in case if a event comes before it should. I think we should relax input machine a bit by not causing panic. In case of send any event to input machine should be just ignored keeping the current state as it is.

Since input machines whole purpose is interactive user input I think it should be bit relaxed. @vu3rdd @warner what do you guys think?

copyninja commented 6 years ago

Alternatively if we intend to keep input machine as is (meaning it will panic) then input machine should also be started when core is started.

copyninja commented 6 years ago

But note that if we try to do Input machine start then we need to integrate with Input machine every where. Else panic just moves to different place :)

warner commented 6 years ago

Yeah, I left a reminder to myself (the TODO comment on want_nameplate()) that turned out to be involved: the wordlist can arrive in states for which we don't want it. In fact, if set_code() is called, it can arrive before we even set the nameplate (although maybe we could fix that, by being more cautious about how we emit the other events).

I'm working on this now. Input should accept (and store) the wordlist in any state. I'm wrestling with whether to implement this by copying all the states (so we'd have a WantNameplateNoNameplatesNoWordlist, WantNameplateNoNameplatesHaveWordlist, WantNameplateHaveNameplatesNoWordlist, etc.. gross), or having a separate Option<Wordlist> on the Input struct (which I think copyninja had recommended before.. you were right :). I'll probably go with the latter.

I'm also changing the Boss machine to have a distinct Start event, because the most immediate problem with io/blocking is that it never initiates the websocket connection, because it isn't currently calling start. Since Start returns Actions, it should really be called in the same way as any other API call (so the caller is prepared to act upon the vector of actions that come back), so I'm wiring it up as a regular API event instead of a distinct API call. For io/blocking, after it sets up the thread and the channel, it'll send() a Start through the API channel, which should kick everything else off.

The application won't see this start event or API: it will be hidden inside the IO layer.

warner commented 6 years ago

I just landed the Start event. I'll work on the Input/wordlist stuff later today.

piegamesde commented 3 years ago

During my refactorings of the core, the input listing feature got cut. Sorry about that. It'll probably have to be re-implemented completely from scratch again, so this issue is obsolete now.