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.

Sign up for GitHub

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

Jump to bottom

feat(deser-lib): add serialization/deserialization module #4095

Merged
merged 16 commits into from
Jan 19, 2024
Merged

feat(deser-lib): add serialization/deserialization module #4095

merged 16 commits into from
Jan 19, 2024

Conversation

johnoliverdriscoll
Copy link
Contributor

Description

Adds serialization / deserialization module. Please see ticket for relevant documents.

Issue Number

HSM-236

Type of change

  • New feature (non-breaking change which adds functionality)

@OttoAllmendinger OttoAllmendinger marked this pull request as draft November 28, 2023 15:41
@socket-security Socket Security
Copy link

socket-security bot commented Nov 29, 2023 •
edited
Loading

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
cbor 9.0.1 None +2 270 kB hildjj

OttoAllmendinger
OttoAllmendinger reviewed Dec 1, 2023
View reviewed changes
Copy link
Contributor

@OttoAllmendinger OttoAllmendinger left a comment

Choose a reason for hiding this comment

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

CI failing - flushing from review queue

zahin-mohammad
zahin-mohammad requested changes Dec 4, 2023
View reviewed changes
modules/deser-lib/package.json Outdated Show resolved Hide resolved
modules/deser-lib/package.json Outdated Show resolved Hide resolved
modules/deser-lib/package.json Outdated Show resolved Hide resolved
modules/deser-lib/package.json Outdated Show resolved Hide resolved
modules/deser-lib/package.json Outdated Show resolved Hide resolved
modules/deser-lib/src/index.ts Outdated Show resolved Hide resolved
modules/deser-lib/src/index.ts Outdated Show resolved Hide resolved
modules/deser-lib/src/index.ts Outdated Show resolved Hide resolved
modules/deser-lib/src/index.ts Outdated Show resolved Hide resolved
modules/deser-lib/src/index.ts Outdated Show resolved Hide resolved
@johnoliverdriscoll johnoliverdriscoll marked this pull request as ready for review December 5, 2023 17:33
zahin-mohammad
zahin-mohammad reviewed Dec 6, 2023
View reviewed changes
Copy link
Contributor

@zahin-mohammad zahin-mohammad left a comment

Choose a reason for hiding this comment

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

Some minor nits, but main comment is regarding the unused getType call Alejandro called out. Is that missed cleanup or is it needed somewhere? If its needed somewhere, lets have a test to show that missing this piece of logic actually fails something.

modules/deser-lib/package.json Show resolved Hide resolved
modules/deser-lib/src/cbor.ts Show resolved Hide resolved
modules/deser-lib/src/cbor.ts Outdated Show resolved Hide resolved
modules/deser-lib/src/cbor.ts Outdated Show resolved Hide resolved
modules/deser-lib/src/cbor.ts Outdated Show resolved Hide resolved
modules/deser-lib/src/cbor.ts Outdated Show resolved Hide resolved
modules/deser-lib/src/cbor.ts Show resolved Hide resolved
modules/deser-lib/test/unit/deser-lib.ts Show resolved Hide resolved
zahin-mohammad
zahin-mohammad previously approved these changes Dec 12, 2023
View reviewed changes
zahin-mohammad
zahin-mohammad previously approved these changes Dec 13, 2023
View reviewed changes
@OttoAllmendinger OttoAllmendinger removed their request for review December 14, 2023 13:15
mohammadalfaiyazbitgo
mohammadalfaiyazbitgo previously requested changes Dec 21, 2023
View reviewed changes
modules/deser-lib/src/cbor.ts Outdated Show resolved Hide resolved
bitgoAaron
bitgoAaron reviewed Jan 9, 2024
View reviewed changes
Copy link
Contributor

@bitgoAaron bitgoAaron left a comment

Choose a reason for hiding this comment

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

how will this package be used by clients of the SDK ? Please avoid adding more dependencies to sdk-core if possible

johnoliverdriscoll and others added 10 commits January 10, 2024 14:44
@zahin-mohammad
Copy link
Contributor

zahin-mohammad commented Jan 10, 2024 •
edited
Loading

still not clear why this is being added to BitGoJS repo if it does not extend or otherwise interact with any of the other packages in this repo. If this is indeed meant to be shipped as part of the SDK, please at least update the CODEOWNERS to include that the HSM team owns this, as well as update the ReadMe for this project explaining what's its used for & how it should be imported/used alongside the SDK.

Just updated both CODEOWNERS and the README, hopefully this clears things up more. Code from other modules will eventually move to this lib to centralize our serialization schemes for tss and multisig. To start we are adding cbor, but the existing schemes we used for tss will eventually move over (or be removed if they are no longer used)

@johnoliverdriscoll
Copy link
Contributor Author

@bitgoAaron Can you please review recent changes amended by @zahin-mohammad

sumanmukherjee03
sumanmukherjee03 previously approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@sumanmukherjee03 sumanmukherjee03 left a comment

Choose a reason for hiding this comment

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

approving the codeowners change

zahin-mohammad
zahin-mohammad previously approved these changes Jan 17, 2024
View reviewed changes
@zahin-mohammad
Copy link
Contributor

Approved, but needs a prettier fix.

bitgoAaron
bitgoAaron previously approved these changes Jan 17, 2024
View reviewed changes
@johnoliverdriscoll
Copy link
Contributor Author

@mohammadalfaiyazbitgo please review changes

@johnoliverdriscoll johnoliverdriscoll merged commit 7a48d4e into master Jan 19, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alebusse alebusse alebusse left review comments

@OttoAllmendinger OttoAllmendinger OttoAllmendinger left review comments

@zahin-mohammad zahin-mohammad zahin-mohammad approved these changes

@sumanmukherjee03 sumanmukherjee03 sumanmukherjee03 approved these changes

@bitgoAaron bitgoAaron bitgoAaron left review comments

@Logicwax Logicwax Awaiting requested review from Logicwax

@mohammadalfaiyazbitgo mohammadalfaiyazbitgo Awaiting requested review from mohammadalfaiyazbitgo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@johnoliverdriscoll @bitgoAaron @zahin-mohammad @sumanmukherjee03 @OttoAllmendinger @Logicwax @alebusse @mohammadalfaiyazbitgo