fix: drop agent EID suffix on multisig OOBI by default - #447
Conversation
372b486 to
7a0c814
Compare
This has been a constant source of confusion for years and should return the intuitive result by default, the multisig Agent OOBI with no agent EID suffix. The includeEid URL parameter allows using the old behavior, if desired.
7a0c814 to
73fbcab
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #447 +/- ##
==========================================
+ Coverage 88.44% 88.61% +0.17%
==========================================
Files 26 27 +1
Lines 5938 5975 +37
==========================================
+ Hits 5252 5295 +43
+ Misses 686 680 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…d-suffix-for-multisig-oobi
| oobis.append(urljoin(up.geturl(), f"/oobi/{hab.pre}/agent/{eid}")) | ||
| res["oobis"] = oobis | ||
| # Outer for loops over multi-valued mict that could have multiple "agent" dict values | ||
| for eurls in roleUrls.naball(kering.Roles.agent): |
There was a problem hiding this comment.
Is this endpoint actually used by Signify, or just /identifiers/{name}/oobis? If not I'd be in favour of removing it and keeping one.
There was a problem hiding this comment.
Yes, it's not used. I'll remove it and move any valuable ideas to /identifiers/{name}/oobis
| hab, url, agent, includeEid=includeEid | ||
| ) | ||
| if oobi not in oobis: | ||
| oobis.append(oobi) |
There was a problem hiding this comment.
A bigger, more breaking change but I think it might be worth considering moving from returning URLs to returning objects and letting the client build the URL.
For example, if we returned objects like {"eid": "EJsP...", "scheme": "http", "url": "http://keria:3902/"} all of the information would be available, and the client decides.
With a default that we build URLs without the eid suffix, as it's not really adding any value beyond specific cases where you want the filtered view (unsure on what those cases are).
There was a problem hiding this comment.
A bigger, more breaking change but I think it might be worth considering moving from returning URLs to returning objects and letting the client build the URL.
I really like that. Let's plan on doing that when we upgrade to 1.0.0. This would allow us to return a rich object and let the client make the decision. Good idea. Please make an issue about this and let's add it to the prioritized roadmap.
There was a problem hiding this comment.
Note that this change is a breaking change, so if we do that, it's 2 breaking changes to this API.
For example, Veridian Wallet right now will search for /agent/ which won't exist on this API (just /agent). Easy fix, but there may be many clients. So if we will put a breaking change, maybe it should be just done once correctly. What do you think?
There was a problem hiding this comment.
Well, maybe we could leave the old API there and make a new endpoint, or a query parameter, that allows returning the object. We could maintain the old API indefinitely and just deprecate it. Then it doesn't have to be breaking.
A breaking change to a core component like this in such a small ecosystem, on second thought, is risky. We could mitigate that risk by allowing an optional upgrade path with default support for the old one.
There was a problem hiding this comment.
Note that this change is a breaking change
I believe that returning a multisig OOBI with an agent EID on the end was a bug, and while stripping the EID by default could be considered a breaking change if someone relied on that, then I am not aware of anyone who actually relied on it.
Technically, yes, it is breaking, yet only for multisig, and only for people that, for whatever reason, needed the filtered view of the single-member's view of the multisig KEL + end roles and loc schemes.
This changes nothing for single sig identifiers and leaves the Agent EID on the end of their OOBIs, so, in my mind, it seems okay to merge.
There was a problem hiding this comment.
For example, Veridian Wallet right now will search for /agent/ which won't exist on this API (just /agent). Easy fix, but there may be many clients.
Oh, I see what you're saying. Darn. I do remember you saying Veridian does not yet do a lot with multisig. Does Veridian do the /agent/ search on multisig identifiers?
So if we will put a breaking change, maybe it should be just done once correctly. What do you think?
Yeah, if we're going to break it, let's do it once. I would still favor having the object being returned be an optional upgrade since we've got a lot of QVI clients at GLEIF that we don't want to break by changing this API.
And that makes me think that maybe we have to support the old /agent/ version indefinitely...hmm.
Or maybe we should just break it once and include an upgrade guide...
I favor breaking it once and forcing people to upgrade so we stop getting so many inadvertent incorrect usages of the API.
There was a problem hiding this comment.
Oh, I see what you're saying. Darn. I do remember you saying Veridian does not yet do a lot with multisig. Does Veridian do the /agent/ search on multisig identifiers?
Veridian has had multi-sig since the start. I'm saying Veridian drops the EID suffix using .indexOf("/agent/");, and it's possible that there are other QVIs which also do this.
And that makes me think that maybe we have to support the old /agent/ version indefinitely...hmm.
While it's worth trying to avoid breaking changes, KERIA is still not even at a 1.0.0 release so one way or another there will be breaking changes. Breaking in a small ecosystem is better than a large one. I also think the Signify APIs are very inconsistent (I know @lenkan thought the same) and need re-working with breaking changes.
We could alternatively maintain a release branch for this pre 1.0.0 version, and put some effort into a more polished API on main. Perhaps something to discuss next Tuesday with a wider group.
There was a problem hiding this comment.
I just realized last night we could also add some intelligence to our OOBI resolution in KERIA that even if a multisig OOBI is resolve we check the AID in the OOBI (not the EID) and if it is a group AID then we would just resolve it as a normal group AID. This would be okay because there are literally zero use cases for resolving the filtered view.
We could add a query param to resolve the filtered view if someone complains and asks for it. Then we can avoid breaking backwards compatibility with any client.
I'll look into this today.
So, this PR could still change the default, yet the old behavior would also be supported.
There was a problem hiding this comment.
Hmm, we could and it would work. It feels like hidden behaviour though and might end up being confusing. I think I would accept it as a non-breaking compromise for now, and move to my suggestion later. Perhaps we can discuss tomorrow
There was a problem hiding this comment.
Yeah, that's my worry, that it is hidden behavior. Compromise is the right description.
Okay, sure, let's discuss tomorrow.
…d-suffix-for-multisig-oobi
2 participants
This has been a constant source of confusion for years and should return the intuitive result by default, the multisig Agent OOBI with no agent EID suffix.
The includeEid URL parameter allows using the old behavior, if desired.
SignifyTS PR: WebOfTrust/signify-ts#400
SignifyPy PR: WebOfTrust/signifypy#150