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

Breaking: Fix publishing multiple rev reg defs with endorsement #3107

Merged
merged 4 commits into from
Jul 18, 2024

Conversation

jamshale
Copy link
Contributor

@jamshale jamshale commented Jul 18, 2024

This fixes a bug/mistake when publishing revocations from multiple rev_reg_defs with endorsement. The endorsement process was occurring outside the manager and only considering the last rev_reg_def pre-endorsement result.

Note: This would be considered a breaking change if a controller was reading and processing the response. It is now an array instead of an object. This should have been the response from the beginning as you can request a list, you should return a list. Could be a nested object instead if that's preferable.

Signed-off-by: jamshale <jamiehalebc@gmail.com>
@jamshale jamshale linked an issue Jul 18, 2024 that may be closed by this pull request
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Copy link

sonarcloud bot commented Jul 18, 2024

@jamshale jamshale marked this pull request as ready for review July 18, 2024 20:02
@jamshale
Copy link
Contributor Author

@swcurran I left a note about the change in the response payload. Should be in the next release docs. Not sure if any controllers would do anything with the response, but they might.

@jamshale jamshale merged commit f3a10ba into hyperledger:main Jul 18, 2024
9 checks passed
@swcurran
Copy link
Member

Sounds good. I’ll put it into the breaking change notes for the release.

@swcurran swcurran changed the title Fix publishing multiple rev reg defs with endorsement Breaking: Fix publishing multiple rev reg defs with endorsement Jul 18, 2024
@cl0ete
Copy link
Contributor

cl0ete commented Aug 8, 2024

@jamshale
Given the response model for publish_revocations:

class TxnOrPublishRevocationsResultSchema(OpenAPISchema):
    """Result schema for credential definition send request."""

    sent = fields.Nested(
        PublishRevocationsSchema(),
        required=False,
        metadata={"definition": "Content sent"},
    )
    txn = fields.Nested(
        TransactionRecordSchema(),
        required=False,
        metadata={
            "description": "Revocation registry revocations transaction to endorse"
        },
    )

Should the model not be updated to handle the fact that txn can now be a list.
And should "rrid2crid" not be txn on line 669 of aries_cloudagent/revocation/routes.py

@jamshale
Copy link
Contributor Author

jamshale commented Aug 8, 2024

I guess I forgot to update the response model here. The response is different depending on whether or not an endorsed transaction was used. If not using endorsement then it had the rrid2crid response. I didn't change this. This endpoint could still be better and have a more consistent response values, but it should work as expected.

I agree the response model should at least be defined as a list. Feel free to open a PR for it or I will get to it shortly.

@ff137
Copy link
Contributor

ff137 commented Aug 9, 2024

@jamshale we'll make a PR for it 👌
Busy testing a fix to the response model with our "cloudcontroller" client, generated from the openapi spec.
(Today's a public holiday here, so will be next week)

darshilnb pushed a commit to Northern-Block/aries-cloudagent-python that referenced this pull request Sep 5, 2024
* Fix publishing multiple rev reg defs with endorsement

Signed-off-by: jamshale <jamiehalebc@gmail.com>

* Refactor / Unit test

Signed-off-by: jamshale <jamiehalebc@gmail.com>

* Add more thourough unit testing

Signed-off-by: jamshale <jamiehalebc@gmail.com>

* Revert auto formatting to avoid doing coverage

Signed-off-by: jamshale <jamiehalebc@gmail.com>

---------

Signed-off-by: jamshale <jamiehalebc@gmail.com>
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.

Revocation: Pending publication bugs
5 participants