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(bbs): extract bbs logic into separate module #1035

Merged
merged 7 commits into from
Oct 7, 2022
Merged

feat(bbs): extract bbs logic into separate module #1035

merged 7 commits into from
Oct 7, 2022

Conversation

TimoGlastra
Copy link
Contributor

Signed-off-by: Timo Glastra timo@animo.id

Fixes #948

Extract the bbs logic into a separate @aries-framework/module-bbs. I had to export a lot of the vc stuff from the core bloating the core imports a bit. I think this will be solved when we extract the vc logic into it's own vc package.

For now I've disabled the bbs tests to run on node 17 and 18, so we will have passing CI again, with the knowledge that BBS is broken in node 17 and 18. I also set the package to private for now so that it won't be released with the 0.3.0 release and we have more time to fix it and get it ready for release.

What do we think of the name @aries-framework/module-bbs? Should we call it @aries-framework/bbs-signatures? It's private for now, so we have room to rename it until we remote the private: true from the package.json

There's is only really ugly hack (one of the first files in the file overview) that has to do with tsyringe injection not allowing nothing to be registered for a token. We have a signing provider that is only used for bbs, and in if you don't register the bbs module it will fail. I didn't want to spend too much time to rework it, so implemented a quick workaround for now

@TimoGlastra TimoGlastra requested a review from a team as a code owner September 22, 2022 10:13
@TimoGlastra TimoGlastra linked an issue Sep 22, 2022 that may be closed by this pull request
Extract bbs dependencies into @aries-framework/bbs-signatures packages #948
Closed
@TimoGlastra
Copy link
Contributor Author

Good to see all tests passing again in 0.3.0 branch :)

@berendsliedrecht
Copy link
Contributor

I prefer the name @aries-framework/bbs-signatures. Mainly for the reason that if we change the name module at some points we won't have to rename our package. It might also make it more loosely coupled with our Aries-framework, however I am not sure how the reusability is of this module. This might also be worthwhile to investigate whether our modules can be resused within other frameworks. And if not, do we want to add this and if so, how?

We might also need a discussion/issue thread where we define all the packages and their names will be. Will didcomm be the core? Which didcomm will be the core? Will there even be a core?

berendsliedrecht
berendsliedrecht approved these changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

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

Just a couple of questions. Nothing really major.

packages/core/src/crypto/WalletKeyPair.ts Outdated Show resolved Hide resolved
packages/core/src/modules/vc/deriveProof.ts Show resolved Hide resolved
packages/module-bbs/package.json
"name": "@aries-framework/module-bbs",
"main": "build/index",
"types": "build/index",
"version": "0.2.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Will all the separate modules be in version-sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes for now all modules in this repo will be version synced

packages/module-bbs/tests/util.ts
@@ -0,0 +1,19 @@
export function testSkipNode17And18(...parameters: Parameters<typeof test>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this prompt an output as to why they are being skipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope... :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if this is something we can add? Maybe just a console.warn giving a small explanation and linking to an issue?

@genaris
Copy link
Contributor

genaris commented Sep 26, 2022

Good to see all tests passing again in 0.3.0 branch :)

So refreshing!

I have no preference with the naming but I agree with @blu3beri that we should add a discussion regarding the modularisation (once we finish this 0.3.0 though) and that it would be appropriate to indicate the reason why we are skipping node 17 and 18 tests. I know we will not forget, as it has prevented us from sleeping for months, but just in case...

Artemkaaas pushed a commit to sicpa-dlab/aries-framework-javascript that referenced this pull request Sep 29, 2022
@spivachuk
openwallet-foundation#1035: Support PartyState and Wallet changes
0a4c1e6
Artemkaaas added a commit to sicpa-dlab/aries-framework-javascript that referenced this pull request Sep 29, 2022
@Artemkaaas
Merge pull request #41 from sicpa-dlab/party-state-and-wallet-changes…
82ee024 
…-support

openwallet-foundation#1035: Support `PartyState` and `Wallet` changes
@jakubkoci
Copy link
Contributor

Just my two cents about the naming, I would rather remove module or package from the name and just use @aries-framework/bbs. We have some struggles with a module name inside the framework, which particular meaning is still to be defined. The fact that it's some sort of package is possible to deduce from the context.

@TimoGlastra TimoGlastra mentioned this pull request Oct 6, 2022
Open
@TimoGlastra
chore: resolve feedback
d570311 
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra
Copy link
Contributor Author

Resolved feedback and changed name to @aries-framework/bbs-signatures

berendsliedrecht
berendsliedrecht approved these changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@berendsliedrecht berendsliedrecht 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 to me!

@TimoGlastra TimoGlastra merged commit 991151b into openwallet-foundation:0.3.0-pre Oct 7, 2022
@TimoGlastra TimoGlastra deleted the feat/bbs-package branch October 7, 2022 11:54
genaris pushed a commit to 2060-io/aries-framework-javascript that referenced this pull request Oct 13, 2022
@TimoGlastra @genaris
feat(bbs): extract bbs logic into separate module (openwallet-foundat…
9ca4221 
…ion#1035)

Signed-off-by: Timo Glastra <timo@animo.id>
@genaris genaris mentioned this pull request Nov 2, 2022
Merged
@berendsliedrecht berendsliedrecht mentioned this pull request Jan 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berendsliedrecht berendsliedrecht berendsliedrecht approved these changes

@genaris genaris Awaiting requested review from genaris

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

Successfully merging this pull request may close these issues.

Extract bbs dependencies into @aries-framework/bbs-signatures packages
4 participants
@TimoGlastra @berendsliedrecht @genaris @jakubkoci