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

Use enums instead of u32 to distinguish connection FSM states on Rust layer #294

Merged
merged 3 commits into from
Jun 15, 2021

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Jun 14, 2021

Replaces u32 representation of AriesFMS states by enums (which do not contain complete copy of state details). u32 representation of FSM state exists only so it could be used on C ABI layer and therefore it should also be expressed in that layer.

Signed-off-by: Patrik Stas patrik.stas@absa.africa

… layer

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas requested a review from a team as a code owner June 14, 2021 11:49
@@ -41,6 +41,13 @@ struct ConnectionInfo {
their: Option<SideConnectionInfo>,
}

#[derive(Debug, PartialEq)]
pub enum ConnectionState {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll be able to remove this enum once we split general Connection into Inviter and Invitee VCX objects, analogically to issuer and holder

@Patrik-Stas
Copy link
Contributor Author

I am sitting on fence whether it wouldn't be better just to return the existing structures (what this PR renamed to InviteeFullState / InviterFullState ) - I was thinking that way the Connection object API would be leaking too much internal details (by calling getState() -> &InviteeFullState, gaining access to call methods directly on the AriesFSM. But giving it a second thought, if it's just a reference to FSM state object and we properly setup field/method visibility, shouldn't do much harm?
What do you think @mirgee

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2021

Codecov Report

Merging #294 (a9bd550) into master (1e199ce) will decrease coverage by 4.16%.
The diff coverage is 37.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
- Coverage   49.81%   45.64%   -4.17%     
==========================================
  Files         153      152       -1     
  Lines       19430    18596     -834     
  Branches     6105     5798     -307     
==========================================
- Hits         9679     8489    -1190     
- Misses       4999     6036    +1037     
+ Partials     4752     4071     -681     
Flag Coverage Δ
integration ?
unittests 45.64% <37.96%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
libvcx/src/lib.rs 0.00% <ø> (ø)
libvcx/src/utils/devsetup_agent.rs 0.00% <0.00%> (-20.33%) ⬇️
libvcx/src/aries/handlers/connection/connection.rs 39.91% <30.76%> (-3.84%) ⬇️
...aries/handlers/connection/invitee/state_machine.rs 61.13% <34.72%> (-1.67%) ⬇️
...aries/handlers/connection/inviter/state_machine.rs 63.07% <40.00%> (+0.25%) ⬆️
libvcx/src/abi_utils/mod.rs 52.94% <52.94%> (ø)
libvcx/src/connection.rs 49.85% <100.00%> (-4.47%) ⬇️
libvcx/src/libindy/utils/pool.rs 14.58% <0.00%> (-52.42%) ⬇️
libvcx/src/libindy/utils/anoncreds.rs 18.04% <0.00%> (-46.61%) ⬇️
libvcx/src/utils/plugins.rs 0.00% <0.00%> (-33.34%) ⬇️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e199ce...a9bd550. Read the comment docs.

@mirgee
Copy link
Contributor

mirgee commented Jun 14, 2021

@Patrik-Stas Awesome, I love this change. As for returning the full state structs instead of just enums, I dislike the idea. I think that the contract between get_state and the caller should be as tight as possible, so that it would less prone to change, and the state machine objects themselves should stay private and internal to the logic of the layers above (Inviter, Invitee, Issuer, Holder, ...).

libvcx/src/aries/handlers/connection/connection.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,28 @@
use crate::aries::handlers::connection::connection::ConnectionState;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about moving this code to src/api/mod.rs, or moving some of the enum conversion stuff to this module, along with src/api/return_types_u32.rs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

happy to do that change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I played a bit and took this suggestion few more steps ahead - I turned it into separate PR https://github.com/hyperledger/aries-vcx/pull/295/files so the original purpose of this one does not get too polluted

Copy link
Contributor

Choose a reason for hiding this comment

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

That's perfect, thank you

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@mirgee mirgee merged commit e96ee50 into master Jun 15, 2021
@mirgee mirgee deleted the refactor/connection-fsm branch June 15, 2021 19:08
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.

None yet

3 participants