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

Remove dependence of aries module on VcxStateType #314

Merged
merged 12 commits into from
Jul 19, 2021

Conversation

mirgee
Copy link
Contributor

@mirgee mirgee commented Jul 1, 2021

This PR adds new enums representing current state of IssuerSM, HolderSM, ProverSM and VerifierSM: IssuerState, HolderState, ProverState and VerifierState respectively. Function get_state() returns variants of said enums on the internal aries level, and these are converted to numeric representations on the C-callable level.

Wrapper now exports enums representing these states in place of StateType: IssuerStateType, HolderStateType, VerifierStateType, IssuerStateType, ConnectionStateType.

This is a breaking change, since the variant representations corresponding to the states returned by get_state() function has changed, as well as the numeric representations. For the numeric values, see here, and for the changes to get_state() return values, see corresponding implementations for each of the concerned SMs.

Signed-off-by: Miroslav Kovar [email protected]

@mirgee mirgee requested a review from a team as a code owner July 1, 2021 18:18
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2021

Codecov Report

Merging #314 (8cadbe3) into master (3c2165d) will increase coverage by 0.04%.
The diff coverage is 35.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
+ Coverage   49.72%   49.77%   +0.04%     
==========================================
  Files         156      156              
  Lines       19634    19651      +17     
  Branches     6205     6199       -6     
==========================================
+ Hits         9763     9781      +18     
- Misses       5070     5074       +4     
+ Partials     4801     4796       -5     
Flag Coverage Δ
integration 22.09% <0.80%> (-0.05%) ⬇️
unittests 45.59% <35.10%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
libvcx/src/api_lib/api_c/credential.rs 27.27% <0.00%> (-0.28%) ⬇️
libvcx/src/api_lib/api_c/credential_def.rs 34.51% <ø> (ø)
libvcx/src/api_lib/api_c/disclosed_proof.rs 34.60% <0.00%> (ø)
libvcx/src/api_lib/api_c/issuer_credential.rs 28.92% <0.00%> (-0.06%) ⬇️
libvcx/src/api_lib/api_c/proof.rs 44.87% <0.00%> (ø)
libvcx/src/api_lib/api_c/wallet.rs 43.66% <ø> (ø)
...aries/handlers/connection/invitee/state_machine.rs 64.50% <0.00%> (ø)
...aries/handlers/connection/inviter/state_machine.rs 64.45% <0.00%> (ø)
libvcx/src/aries/mod.rs 23.07% <ø> (ø)
libvcx/src/lib.rs 0.00% <0.00%> (ø)
... and 19 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 3c2165d...8cadbe3. Read the comment docs.

@mirgee mirgee force-pushed the refactor/remove-vcxstatetype branch from 249b63e to bda18f2 Compare July 1, 2021 21:06
Signed-off-by: Miroslav Kovar <[email protected]>
@mirgee mirgee force-pushed the refactor/remove-vcxstatetype branch from bda18f2 to 03cd4e5 Compare July 1, 2021 21:17
@Patrik-Stas
Copy link
Contributor

Patrik-Stas commented Jul 2, 2021

@mirgee the failing tests seem to indicate that the state mapping is a bit off somewhere

Signed-off-by: Miroslav Kovar <[email protected]>
@mirgee mirgee force-pushed the refactor/remove-vcxstatetype branch from 0287e2d to 838d8e6 Compare July 2, 2021 12:02
ci/indy-pool.dockerfile Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@Patrik-Stas Patrik-Stas left a comment

Choose a reason for hiding this comment

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

Minor changes requested, overally love it! 👍

@mirgee mirgee force-pushed the refactor/remove-vcxstatetype branch 2 times, most recently from ff74561 to 0e2815b Compare July 8, 2021 16:33
Signed-off-by: Miroslav Kovar <[email protected]>
@mirgee mirgee force-pushed the refactor/remove-vcxstatetype branch from 0e2815b to 0fdbe5b Compare July 8, 2021 18:20
Copy link
Contributor

@Patrik-Stas Patrik-Stas left a comment

Choose a reason for hiding this comment

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

I think it's important for readibility of the code consuming this wrapper to return Struct/Enums from TS function returning state representation (getState, updateState)

@Patrik-Stas Patrik-Stas merged commit 6d46888 into master Jul 19, 2021
@Patrik-Stas Patrik-Stas deleted the refactor/remove-vcxstatetype branch July 19, 2021 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants