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

🐛 Ensure supported DID before calling Rotate #3380

Merged
merged 11 commits into from
Dec 17, 2024

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Dec 5, 2024

Resolves #3379 and #3381


I'm open to alternatives. I first thought to just call DID.validate, and raise BadRequest if that fails, but since ensure_supported_did is called downstream, and the client would not immediately know if the DID is unresolvable, I figured that everything may as well be validated in the route, before being sent to the RotateHandler.

That means ensure_supported_did will now be called twice. Perhaps the 2nd call can now be removed, since it's now just called earlier. Open to suggestions

@ff137 ff137 marked this pull request as draft December 5, 2024 10:36
@ff137
Copy link
Contributor Author

ff137 commented Dec 5, 2024

My attempted solution, when passing a qualified did sov, results in:

  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/protocols/did_rotate/v1_0/routes.py", line 73, in rotate
    await did_rotate_mgr.ensure_supported_did(to_did)
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/protocols/did_rotate/v1_0/manager.py", line 242, in ensure_supported_did
    await resolver.resolve(self.profile, did)
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/resolver/did_resolver.py", line 87, in resolve
    _, doc = await self._resolve(profile, did, service_accept, timeout=timeout)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/resolver/did_resolver.py", line 63, in _resolve
    document = await asyncio.wait_for(
               ^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/asyncio/tasks.py", line 520, in wait_for
    return await fut
           ^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/resolver/base.py", line 161, in resolve
    result = await self._resolve(profile, did, service_accept)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/resolver/default/indy.py", line 185, in _resolve
    vmethod = builder.verification_method.add(
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/pydid/doc/builder.py", line 50, in add
    vmethod = type_.make(id=self._did.ref(ident), controller=controller, **kwargs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/pydid/resource.py", line 101, in make
    return cls(**kwargs)
           ^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/pydid/verification_method.py", line 57, in __init__
    super().__init__(**data)
  File "/home/aries/.local/lib/python3.12/site-packages/pydantic/main.py", line 214, in __init__
    validated_self = self.__pydantic_validator__.validate_python(data, self_instance=self)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
pydantic_core._pydantic_core.ValidationError: 1 validation error for Ed25519VerificationKey2018
public_key_base58
  Input should be a valid string [type=string_type, input_value=None, input_type=NoneType]

...

Seems this is the scenario for a did:sov that's not posted to the ledger

Everything seems to work fine for did:peer:2 and did:peer:4. did:key returns an unresolvable didcomm service problem report. Will investigate more, and try improve some things

@ff137
Copy link
Contributor Author

ff137 commented Dec 5, 2024

RotateAck is not correctly configured. Somewhere it is trying to load class "Ack", instead of RotateAck:

"/home/aries/.local/lib/python3.12/site-packages/acapy_agent/utils/classloader.py", line 234, in resolved
    DeferLoad._class_cache[self._cls_path] = ClassLoader.load_class(
                                             ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/utils/classloader.py", line 125, in load_class
    raise ClassNotFoundError(
acapy_agent.utils.classloader.ClassNotFoundError: Class 'Ack' not defined in module: acapy_agent.protocols.did_rotate.v1_0.messages.ack

Found it here: 6bf8fc8

@ff137 ff137 linked an issue Dec 6, 2024 that may be closed by this pull request
@ff137 ff137 marked this pull request as ready for review December 11, 2024 09:08
@ff137 ff137 requested review from dbluhm and jamshale December 11, 2024 09:18
@ff137
Copy link
Contributor Author

ff137 commented Dec 11, 2024

@jamshale @dbluhm just a headsup, it looks like the SonarCloud coverage analysis is broken. My coverage report locally shows all changes are covered, but SonarCloud gives 0%. Presumably relates to #3390

@jamshale
Copy link
Contributor

@jamshale @dbluhm just a headsup, it looks like the SonarCloud coverage analysis is broken. My coverage report locally shows all changes are covered, but SonarCloud gives 0%. Presumably relates to #3390

I'll fix this. Something changed with github actions recently that broke this.

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.

Overall looks pretty good. Quick nitpick on handling errors raised by pydid.

acapy_agent/connections/base_manager.py Outdated Show resolved Hide resolved
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.

👍

@swcurran swcurran merged commit e3b0841 into openwallet-foundation:main Dec 17, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants