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

fix: always use Agent proxy when sending messages from SignifyGroupHab #353

Merged

Conversation

kentbull
Copy link
Contributor

@kentbull kentbull commented Jan 22, 2025

This fixes delegated multisig rotation with KERIA and Signify multisig groups. Prior to this fix then sending a delegated multisig rotation would always trigger a kering.KeriError as shown below because the existing if else chain sets `phab = hab as shown below.

First, the def sign in SignifyHab throwing the KeriError.

class SignifyHab(BaseHab):
    ...
    def sign(self, ser, verfers=None, indexed=True, indices=None, ondices=None, **kwa):
        ...
        raise kering.KeriError("Signify hab does not support local signing")

Next, the if/else chain in keria.app.delegating.Anchorer.processPartialWitnessEscrow

class Anchorer:
    ...
    def processPartialWitnessEscrow(...):
        ...
                if isinstance(hab, habbing.GroupHab):
                    phab = hab.mhab
                elif hab.kever.sn > 0:
                    phab = hab  # <-- this branch was being incorrectly called because for SignifyGroupHab instances where the delegate has undergone a rotation result in this branch evaluating to true, preventing the check for the self.proxy.
                elif self.proxy is not None:
                    phab = self.proxy
        ...

The new code causes this Anchorer to always check for a proxy and use it if specified:

                if isinstance(hab, habbing.GroupHab):
                    phab = hab.mhab
                elif self.proxy is not None:
                    phab = self.proxy
                elif hab.kever.sn > 0:
                    phab = hab

Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.78%. Comparing base (ab20766) to head (9e48c70).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/keria/app/delegating.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
+ Coverage   93.71%   93.78%   +0.06%     
==========================================
  Files          37       37              
  Lines        8293     8415     +122     
==========================================
+ Hits         7772     7892     +120     
- Misses        521      523       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This fixes delegated multisig rotation with KERIA and Signify multisig groups.
@kentbull kentbull force-pushed the delegated-multisig-rotation-proxy-fix branch from 4043c89 to 9e48c70 Compare January 22, 2025 22:30
@kentbull kentbull requested review from m00sey, 2byrds and iFergal January 22, 2025 22:46
@kentbull
Copy link
Contributor Author

Hey all, can we relax the code coverage for this patch since we don't yet have any signify tests in this repo? I will add a test to the SignifyPy and SignifyTS repo for this later next week or so. I have a Bash script integration test in GLEIF-IT/qvi-software as a start.

@kentbull
Copy link
Contributor Author

I checked with Phil just now and he said moving these logic branches around are okay because you can always use the proxy when present except when there is a GroupHab.

Copy link
Collaborator

@2byrds 2byrds left a comment

Choose a reason for hiding this comment

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

We reviewed this in the vLEI dev community meeting. Kent explained the discussions he had with Phil. And this seems to be an appropriate workaround. There is a desire for better tests for these kind of changes. And some of the signifypy related tests would help here.

@2byrds 2byrds merged commit 87a1e42 into WebOfTrust:main Jan 23, 2025
4 of 5 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
Development

Successfully merging this pull request may close these issues.

2 participants