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

PublishRevocations model replaced with PublishRevocationsSchemaAnoncreds #2901

Closed
ff137 opened this issue Apr 18, 2024 · 2 comments · Fixed by #2909
Closed

PublishRevocations model replaced with PublishRevocationsSchemaAnoncreds #2901

ff137 opened this issue Apr 18, 2024 · 2 comments · Fixed by #2909
Assignees

Comments

@ff137
Copy link
Contributor

ff137 commented Apr 18, 2024

Good day

I notice from working with the openapi spec, in 0.12.0 there is a new model: PublishRevocationsSchemaAnoncreds.
It looks like this (from running openapi-generator):

class PublishRevocationsSchemaAnoncreds(BaseModel):
    rrid2crid: Optional[Dict[str, List[Annotated[str, Field(strict=True)]]]] = Field(
        default=None, description="Credential revocation ids by revocation registry id"
    )

This new model is now used to replace the previous PublishRevocations models in the Revocation API:

  • The following methods: clear_pending_revocations and publish_revocations
  • they now return PublishRevocationsSchemaAnoncreds instead of the old PublishRevocations
  • Additionally, the TxnOrPublishRevocationsResult model, which was previously using PublishRevocations, is now also using PublishRevocationsSchemaAnoncreds, which seems to be an unnecessary change.

Now, the confusion set in when I noticed the Anoncreds Revocation API is using the old PublishRevocations, instead of the new PublishRevocationsSchemaAnoncreds. The naming suggests that this is an oversight.

The old model was modified to now take extra options, which the PublishRevocationsSchemaAnoncreds does not.

class PublishRevocations(BaseModel):
    options: Optional[PublishRevocationsOptions] = None
    rrid2crid: Optional[Dict[str, List[Annotated[str, Field(strict=True)]]]] = Field(
        default=None, description="Credential revocation ids by revocation registry id"
    )

So, my guess is that PublishRevocationsSchemaAnoncreds is the model that should be used in the new AnoncredsRevocationApi, and that this is the model that should have the extra options field, not the old one.

All in all, this isn't problematic for the internals of ACA-Py, because the models are identical, it's just a difference in names, with the extra options field. But, the model names propagate to the openapi spec, which is causing some confusion!

If my observations are correct that this is an oversight, then do let me know and I can contribute a quick renaming fix.

@ff137
Copy link
Contributor Author

ff137 commented Apr 18, 2024

Additionally, I notice that the old Revocation API, in the get_revocation_status method, it replaces CredRevRecordResult with CredRevRecordResultSchemaAnoncreds. I assume this is also an oversight and that the Revocation API should use the same models as previous; that only the AnoncredsRevocationApi should be using these new models instead.

The old CredRevRecordResult uses the new result type: IssuerCredRevRecordSchemaAnoncreds ...

class CredRevRecordResult(BaseModel):
    result: Optional[IssuerCredRevRecordSchemaAnoncreds] = None

with the new CredRevRecordResultSchemaAnoncreds using the old result ...

class CredRevRecordResultSchemaAnoncreds(BaseModel):
    result: Optional[IssuerCredRevRecord] = None

Looks like someone used find-replace-all a bit too liberally! But idk, maybe there's method behind it?

@jamshale
Copy link
Contributor

jamshale commented Apr 22, 2024

I likely made a mistake here. I'll have a look. Thanks for spotting it.

The additional names were added because we are loading the schemas for both anoncreds and the existing modules and they conflicted. They are similar but slightly different, and I think you're right, I mixed them up a bit.

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 a pull request may close this issue.

2 participants