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

Feature multikey management #3246

Merged
merged 28 commits into from
Sep 24, 2024
Merged

Feature multikey management #3246

merged 28 commits into from
Sep 24, 2024

Conversation

PatStLouis
Copy link
Contributor

@PatStLouis PatStLouis commented Sep 20, 2024 •
edited
Loading

Supersedes #3168.

During the last aca-py call, it was discuss that it would be preferable to create keys instead of dids for associating a verification method.

This PR adds 3 routes to the wallet and adds an optional kid property to the KeyInfo class.

Deleting a key should be looked at in a separate PR as this feature isn't currently supported by aca-py and could have other ramifications to be taken into account.

@dbluhm @jamshale

@PatStLouis
Copy link
Contributor Author

@jamshale I'm having issues with the linting tests. I have ruff version 0.6.5 and even if I apply formatting it's still causing errors without much details. Do you have a clue what I'm doing wrong here?

@jamshale
Copy link
Contributor

I think it's working correctly. Sometimes the formatter won't be able to automatically fix things.

image

The line too long is annoying sometimes. I'm fine with just adding # noqa: E501 to these lines. The others just need doc string and we have a preference to have is None instead of == None.

Usually I just run poetry run ruff check . when I get a fail. There should be a vscode run option for the devcontianer as well.

@PatStLouis
linting fix
a1731d4 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
Copy link
Contributor Author

now it's asking me to use version ruff==0.5.7 but the pyproject.toml lists 0.6.5

creating virtual environment...
installing ruff from spec 'ruff==0.5.7'...
Would reformat: aries_cloudagent/wallet/did_info.py
1 file would be reformatted, 1360 files already formatted

@PatStLouis
more linting
e6bbe50 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
Copy link
Contributor Author

ok I think I sorted it out this time!

@PatStLouis
add 2 more tests
87fdcca 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
linting
6d1257c 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
await function in tests
8210b3d 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
Copy link
Contributor Author

@jamshale @dbluhm I'm happy with the state of this PR, every new function has a test

jamshale
jamshale reviewed Sep 22, 2024
View reviewed changes
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

LGTM. I'm going to let Daniel have a look before approving.

aries_cloudagent/wallet/keys/manager.py Outdated Show resolved Hide resolved
aries_cloudagent/wallet/keys/manager.py Outdated Show resolved Hide resolved
aries_cloudagent/wallet/keys/routes.py Outdated Show resolved Hide resolved
@PatStLouis
askar support
942fdbf 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
implement askar
f62c580 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
spelling mistake
1fcd715 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
use a constant for default algorithm
d94a41d 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
linting
427722f 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
Copy link
Contributor Author

Upon further testing I noticed that for it to be compatible with the askar wallet there was some slight changes required. I've implemented those.

One issue is Askar is unable for find the key given a kid. Has the get_key_by_kid function been tested with the askar wallet? If so, could someone point me towards where it's used in the code base? The only place I can find it is in the test_in_memory_wallet.py file.

@PatStLouis
remove unused code
e632b57 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
formatting
3034ad8 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
linting
41fa48d 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
fix key by kid
9249ec5 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
Copy link
Contributor Author

ok I got it to work as I intended

@PatStLouis
fix unit tests
b6f837a 
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
Copy link
Contributor Author

@jamshale can you add @dbluhm as a reviewer here?

@jamshale jamshale requested a review from dbluhm September 24, 2024 14:47
@jamshale
Copy link
Contributor

Done. One thing I was wondering was if we can or should do a scenario integration test for this? They are a lot quicker and easier to write than before.

dbluhm
dbluhm requested changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

Some recommended changes

aries_cloudagent/wallet/keys/manager.py Outdated Show resolved Hide resolved
aries_cloudagent/wallet/keys/manager.py Outdated Show resolved Hide resolved
aries_cloudagent/wallet/keys/manager.py Outdated Show resolved Hide resolved
aries_cloudagent/wallet/keys/manager.py Show resolved Hide resolved
aries_cloudagent/wallet/keys/manager.py Show resolved Hide resolved
aries_cloudagent/wallet/keys/manager.py Outdated Show resolved Hide resolved
aries_cloudagent/wallet/keys/manager.py Show resolved Hide resolved
@PatStLouis
Copy link
Contributor Author

@dbluhm I've implemented all your recommendations, @jamshale I will have a quick look

dbluhm
dbluhm reviewed Sep 24, 2024
View reviewed changes
Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

Sorry, one more type issue to report that I spotted while reviewing your new changes.

aries_cloudagent/wallet/keys/manager.py Outdated Show resolved Hide resolved
@PatStLouis
Copy link
Contributor Author

@jamshale @dbluhm if we are happy with the state of this work I would like to have it merged sooner than later to start re basing #3181

jamshale
jamshale reviewed Sep 24, 2024
View reviewed changes
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

Just one comment and I'll approve.

@PatStLouis PatStLouis requested a review from jamshale September 24, 2024 18:08
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
67.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@jamshale jamshale merged commit d3644da into openwallet-foundation:main Sep 24, 2024
10 of 11 checks passed
@PatStLouis PatStLouis mentioned this pull request Sep 24, 2024
Closed
@PatStLouis PatStLouis deleted the feature-multikey-management branch October 17, 2024 03:57
ff137 pushed a commit to ff137/acapy that referenced this pull request Oct 22, 2024
@PatStLouis @ff137
Feature multikey management (openwallet-foundation#3246)
89120be 
* load key plugin

Signed-off-by: Patrick <[email protected]>

* simplify functions

Signed-off-by: Patrick <[email protected]>

* added option kid field to KeyInfo

Signed-off-by: PatStLouis <[email protected]>

* improving linting and unit tests

Signed-off-by: PatStLouis <[email protected]>

* add empty kid value for bbs tests

Signed-off-by: PatStLouis <[email protected]>

* linting fix

Signed-off-by: PatStLouis <[email protected]>

* more linting

Signed-off-by: PatStLouis <[email protected]>

* add 2 more tests

Signed-off-by: PatStLouis <[email protected]>

* linting

Signed-off-by: PatStLouis <[email protected]>

* await function in tests

Signed-off-by: PatStLouis <[email protected]>

* askar support

Signed-off-by: PatStLouis <[email protected]>

* implement askar

Signed-off-by: PatStLouis <[email protected]>

* spelling mistake

Signed-off-by: PatStLouis <[email protected]>

* use a constant for default algorithm

Signed-off-by: PatStLouis <[email protected]>

* linting

Signed-off-by: PatStLouis <[email protected]>

* remove unused code

Signed-off-by: PatStLouis <[email protected]>

* formatting

Signed-off-by: PatStLouis <[email protected]>

* linting

Signed-off-by: PatStLouis <[email protected]>

* fix key by kid

Signed-off-by: PatStLouis <[email protected]>

* fix unit tests

Signed-off-by: PatStLouis <[email protected]>

* add type hints and pass session to call functions instead of profile

Signed-off-by: PatStLouis <[email protected]>

* remove manager from test function instanciation

Signed-off-by: PatStLouis <[email protected]>

* replace inject_or with inject for providing wallet interface

Signed-off-by: PatStLouis <[email protected]>

* move wallet injection to class initialization step

Signed-off-by: PatStLouis <[email protected]>

---------

Signed-off-by: Patrick <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbluhm dbluhm dbluhm approved these changes

@jamshale jamshale Awaiting requested review from jamshale

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

Successfully merging this pull request may close these issues.
3 participants
@PatStLouis @jamshale @dbluhm