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

Simplify connection serialization from nodejs #749

Merged
merged 7 commits into from
Feb 14, 2023

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Feb 10, 2023

  • NodeJS wrapper was previously trying to keep value of source_id as part of constructed JS objects / TS classes, see changes in class VcxBase
protected _sourceId: string;
  • Consequently deserialization constructors requiring source_id. That led me to generalizing signature for its static method _deserialize such the serialized data to be deserialized are simply a JSON object (expressed as Record<string, unknown> in TS) instead of previous more strict definition objData: ISerializedData<{ source_id: string }> which couldn't acommodate different serialization format of Connection state machine.
  • Additionally removed some dead code
  • Note: More code can be purged and cleaned up, for example given the decision to treat serialized data as opaque, it makes no sense to define ISerializedData, as this is expression of certain assumption as to how that serialized data looks like. The only thing which should be held by TS classes is the numeric rust handle
  • Simplifies Connection serialization such that in serialized to (removing extra data level in hierarchy, removes duplicated field source_id, removes version)
{
    "source_id": "",
    "pairwise_info": {
        "pw_did": "FhrSrYtQcw3p9xwf7NYemf",
        "pw_vk": "91qMFrZjXDoi2Vc8Mm14Ys112tEZdDegBZZoembFEATE"
    },
    "state": {
        "Inviter": {
            "Initial": null
        }
    }
}

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

@Patrik-Stas Patrik-Stas marked this pull request as draft February 10, 2023 11:50
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Merging #749 (6309949) into main (66c8d65) will increase coverage by 7.38%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #749      +/-   ##
==========================================
+ Coverage   47.27%   54.66%   +7.38%     
==========================================
  Files         381      381              
  Lines       32908    37155    +4247     
  Branches     7052     8085    +1033     
==========================================
+ Hits        15558    20310    +4752     
+ Misses      12700    10892    -1808     
- Partials     4650     5953    +1303     
Flag Coverage Δ
unittests-aries-vcx 54.55% <ø> (+7.29%) ⬆️

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

Impacted Files Coverage Δ
aries_vcx/src/protocols/common.rs 76.92% <0.00%> (-23.08%) ⬇️
aries_vcx/src/protocols/oob/mod.rs 72.22% <0.00%> (-11.12%) ⬇️
aries_vcx/src/indy/anoncreds.rs 71.73% <0.00%> (-5.19%) ⬇️
aries_vcx/src/utils/serialization.rs 0.00% <0.00%> (ø)
...s_vcx/src/handlers/connection/legacy_agent_info.rs 0.00% <0.00%> (ø)
...cols/mediated_connection/invitee/states/invited.rs 50.00% <0.00%> (ø)
...cols/mediated_connection/inviter/states/initial.rs 70.00% <0.00%> (ø)
...ols/mediated_connection/inviter/states/complete.rs 58.33% <0.00%> (ø)
libvdrtools/src/services/pool/pool.rs 48.23% <0.00%> (+0.18%) ⬆️
libvdrtools/src/services/ledger/mod.rs 29.15% <0.00%> (+0.57%) ⬆️
... and 114 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@bobozaur bobozaur left a comment

Choose a reason for hiding this comment

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

Not related to the code itself, but rather to the base branch.
I think it would be best to merge #739 and merge this into main afterwards.

The current state of #739 behaves exactly the way main currently does. This PR changes that, which is why I think it deserves it's separate spot for code history's reasons.

@Patrik-Stas Patrik-Stas force-pushed the nodejs/connection-serialization branch from ab86492 to e6e7716 Compare February 13, 2023 11:44
Base automatically changed from refactor/typestate_connections to main February 13, 2023 14:12
@Patrik-Stas Patrik-Stas force-pushed the nodejs/connection-serialization branch 2 times, most recently from 3a68c32 to 70fc9c7 Compare February 13, 2023 18:27
… 22 hosts

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas force-pushed the nodejs/connection-serialization branch from 6309949 to 13fff79 Compare February 13, 2023 19:29
@Patrik-Stas Patrik-Stas changed the base branch from main to ci/nodejs February 13, 2023 19:34
@Patrik-Stas Patrik-Stas requested a review from bobozaur February 14, 2023 07:50
Copy link
Contributor

@bobozaur bobozaur left a comment

Choose a reason for hiding this comment

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

Looks good!

Base automatically changed from ci/nodejs to main February 14, 2023 09:43
export interface IInitVCXOptions {
libVCXPath?: string;
}

export interface ISerializedData<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: we agreed to remove the assumptions this interface makes about the serialization format (i.e. containing source_id, data, version or any other fields, but the assumption of it being a JSON will stay for now).

@mirgee mirgee merged commit b1dbbe2 into main Feb 14, 2023
@mirgee mirgee deleted the nodejs/connection-serialization branch February 14, 2023 11:32
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.

4 participants