Refactor FxA state machine to simplify transitions#7359
Conversation
skhamis
force-pushed
the
fxa-state-machine-refactor
branch
from
6b5e283 to
bd490c0
Compare
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
I like this approach a lot, it feels way more direct than having the internal states implicitly trigger a FirefoxAccount method call. I just left some comments/questions about the details.
| impl StateMachineErr { | ||
| /// Programming errors (logic / invalid-transition) become `Fatal` and | ||
| /// ignore `target_if_handled`; everything else becomes `Handled`. | ||
| pub fn from_cause(cause: Error, target_if_handled: FxaState) -> Self { |
There was a problem hiding this comment.
Nit: StateMachineErr:from_cause() and Result<T, StateMachineError>::err_state() do pretty much the same thing but have very different names. I can't think of a good single name, but maybe we could get a pair of names that seem related like StateMachineErr::new() and Result<T, StateMachineError>::map_err_to_state_machine_err() . I don't really love those names either though, so whatever you want to go with is good with me.
There was a problem hiding this comment.
ah good callouts, I'll go to ::new() as that feels the most "rust-y". I feel like for chaining on the result maybe going to_state_machine_err has the strongest readability imo.
| let scope_refs: Vec<&str> = scopes.iter().map(String::as_str).collect(); | ||
| let oauth_url = account | ||
| .begin_oauth_flow(&service, &scope_refs, &entrypoint) | ||
| .err_state(|| S::Disconnected)?; |
There was a problem hiding this comment.
I think there could be a default handler that would keep the user on their current state. Would that be better than having to write out the err_state for every call? I'm not sure, but I think I like having to write out the error handling logic explicitly.
There was a problem hiding this comment.
Yeah this is a tough one, I do agree with writing out the error handling logic explicitly cause you kinda need to think about "what does this mean and where to do we go", but I think keeping the user in the current state as default might bite us later and I don't think we'll be adding that many states for FxA that we'd need to be ergonomic about it? Happy to still continue if we think so.
There was a problem hiding this comment.
Nope, I was just thinking out loud and wondering what you thought. The current design is good for me, let's keep using it.
skhamis
force-pushed
the
fxa-state-machine-refactor
branch
from
bd490c0 to
2ceb842
Compare
2 participants
Now that FxiOS has the new state machine, we're able to now re-look at the state machine system as a whole and we realized we can simplify it now that we're not merging with existing systems on the consumers. My main goals were:
@bendk had to manage two state machines in android which was the reason for the initial system, but now that they've migrated fully over, the idea came that we can remove the internal state completely and move to a flat-state-machine style. It's in draft now as I think this is a strong denature and want to get thoughts on this before moving on.
Pull Request checklist
[ci full]to the PR title.