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: a way to prevent duplicated anchor events for multisig issuance #286

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lenkan
Copy link
Collaborator

@lenkan lenkan commented Oct 16, 2024

This PR solves an issue that occurs when a multisig member is trying to join an issuance event after it has been fully signed. They need to do this to be able to import the credential in their database.

This is an alternative approach to the PR by @rodolfomiranda #271.

I will do the same for credentials().revoke() and registries().create() if you agree with this approach.

With these changes, I am able to complete this test case: https://github.com/nordlei/vlei-sandbox/blob/main/src/issues/multisig-catchup-problem.test.ts, so we should be able to resolve this issue: WebOfTrust/keria#283. The catch up mechanism would be to simply join all notifications for the multisig group.

@lenkan lenkan force-pushed the fix-credential-issuance-catchup branch from 25215bf to 793f9b9 Compare October 16, 2024 10:46
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.58%. Comparing base (32ceda1) to head (d2278e5).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
+ Coverage   83.55%   83.58%   +0.02%     
==========================================
  Files          48       48              
  Lines        4215     4221       +6     
  Branches     1055     1047       -8     
==========================================
+ Hits         3522     3528       +6     
+ Misses        689      663      -26     
- Partials        4       30      +26     

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

const sn = new CesrNumber({}, parseInt(hab.state.s, 16) + 1);
const anc = new Serder(
args.anc ??
Saider.saidify({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: the reason I inlined this is because I think it makes it much more obvious what we are actually posting down to KERIA. I do not think the indirection of calling interact provides any readability improvements. But I am OK with either. Just provide your feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good with me

@lenkan
Copy link
Collaborator Author

lenkan commented Oct 16, 2024

In the code, I have found several patterns for API operations that are posting Key Events.

IPEX Approach (e.g. apply and submitApply)

apply(): ApplyEventWithSignatures {
   // Create serders and sigs
}

async submitApply() {
   // Do fetch call
}

That approach would work here as well.

End role approach

Here we separate the creation of events into a private method to create the events named "makeXXXEvent". For example:

private makeEndRole() {}
async addEndRole() {
    const rpy = this.makeEndRole()
    // Do fetch call
}

Create from events approach

For the registry creation, we have a pattern of createFromEvents where the actual fetch call is extracted to a separate private method:

Notes

I still think I prefer the method that I did in this PR. But I think we can discuss and decide on one pattern and implement that for all calls that involve posting key events. You can always create external helper methods to create the XXXArgs object needed.

@iFergal
Copy link
Contributor

iFergal commented Oct 16, 2024

@lenkan For IPEX the advantage of how it is set up is that you can also send /multisig/exn with an embedded /ipex/apply using submitApply. KERIA will just handle them differently based on the route.

I kind of like that approach as it's a bit more flexible in general and quite clear from the business logic level: a clean function to create a message or event, and the API submission. This would be my preference in general.

But your change in this PR also makes sense to me if that's preferred by people. I don't like jumping too many levels deep in function calls as it's easy to get lost.

@2byrds
Copy link
Contributor

2byrds commented Nov 15, 2024

@lenkan For IPEX the advantage of how it is set up is that you can also send /multisig/exn with an embedded /ipex/apply using submitApply. KERIA will just handle them differently based on the route.

I kind of like that approach as it's a bit more flexible in general and quite clear from the business logic level: a clean function to create a message or event, and the API submission. This would be my preference in general.

But your change in this PR also makes sense to me if that's preferred by people. I don't like jumping too many levels deep in function calls as it's easy to get lost.

@lenkan what do you think about advantages that @iFergal lists here?

Copy link
Contributor

@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.

@lenkan I updated my signify-ts to this branch. And then tried to modify vlei-sandbox to use the new IssueCredentialArgs interface. But perhaps you already did this and can share? if i can prove that your catchup test works, i can include it in my rollback to prove that member3 is operational in the group again

@iFergal
Copy link
Contributor

iFergal commented Nov 18, 2024

I discussed this offline with @lenkan a while ago. It is a bit curious that there's "2 ways" to do multi-sig IPEX which is probably unnecessary complexity. The benefit is less network calls though.

Though based on the eventual outcome of WebOfTrust/keripy#884 it might need to have the calls split anyway.

@lenkan
Copy link
Collaborator Author

lenkan commented Nov 19, 2024

@lenkan I updated my signify-ts to this branch. And then tried to modify vlei-sandbox to use the new IssueCredentialArgs interface. But perhaps you already did this and can share? if i can prove that your catchup test works, i can include it in my rollback to prove that member3 is operational in the group again

Hm, when I tried that, I only used npm link locally, so I never had that branch of vlei-sandbox pushed. Let me know if it is still an issue, then I could probably push a branch to vlei-sandbox that uses this for joining the credential.

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.

3 participants