-
Notifications
You must be signed in to change notification settings - Fork 516
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
PleaseAck.md document #2540
PleaseAck.md document #2540
Conversation
Signed-off-by: Alexander Sukhachev <[email protected]>
0e577ab
to
ef1b91c
Compare
created a PoC (#2546) for the |
Signed-off-by: Alexander Sukhachev <[email protected]>
3cbc45b
to
e651b93
Compare
Outstanding document and assessment. Great work! Sorry it has taken so long for feedback. I agree with your assessment on the options, and that 2 and 3 are preferred, with the choice hinging on the question: do we need to support the please_ack in all messages? Let’s discuss this at the ACA-Pug to seek other opinions, but my view would be that we go with Option 2 (since it appears to be the easiest) and that we limit the implementation to the relevant protocols in AIP 2.0. In my reading that would include, at most:
Assuming the ACK requested is different from any other Ack in the protocol, we may want to ignore the duplication of sending ACKs. A relevant question for me is whether a controller is able to decide on a message by message basis whether or not to respond to Please Ack requests. There could be configuration settings “Always”, “Never”, but should we allow a setting “Maybe” where the controller is given the option on each call? I lean towards no, but... |
Hello @swcurran. Thank you for the review. Answering your questions I think options "Never"/"Maybe" may be a problem if we speak about The documentation says
It means that an Issuer will not reach the |
Good find @alexsdsr — thanks. Agreed! And it is much easier! |
After reviewing the 0317 Please Ack RFC and your excellent investigation into the protocol and the state of ACA-Py, here are my comments and suggestions:
What do you think? Does that make sense to you, given the investigation you have done? |
Hello @swcurran. Thanks for your comments.
I think we should answer to the question "should we handle the ack message that is received in response to the
i. Your idea is to consider the ii. As I have mentioned previously, I see that protocols declare where the
Unfortunately, nothing directly said about the Holder's behavior, but it seems like it is implied that Holder sends either one ACK (if the |
Good points. Here are my thoughts after reviewing:
If we go with the I think there is also a good argument that a protocol based OUTCOME ACK is most important, as once the “outcome” of a message is known, a message is sent to convey that to the other side(s). However, some protocols (such as "Present Proof v2.0” does not send a finishing ACK from the verifier to the holder and so is a great candidate for this decorator. We can talk it to the Aries community, but I would say that we should go with protocol OUTCOME only.
Let’s see if we can get to a brief summary of the answers and then talk to the broader Aries community. |
Hello @swcurran. There are some answers and questions below.
Since the
That means may be we should choose between option 1 (if we are sure that protocol-bound
Yes, "no state impact" is only possible for the
I see that, at least,
ACA-py finds a handler for message using special Disadvantages of ACK message adoptation:
Disadvantages of using original ACK messages:
Actually, we need to use adopted messages in case of One more thing I would like to discuss is compatibility issue. I have mentioned it here. What is the problem? Let's assume an issuer agent is based on current version of code but a holder agent is based on new version of code (where Then the issuer sends credentials and waits for an ACK (it is always sent in current version of ACA-py). But the holder doesn't send the ACK message because the I see three possible options:
Disadvantages:
Disadvantages:
I think option 1 (to send an ACK always to keep compatibility) may be a good choice for the first version of |
Hello @swcurran
Have you had a chance to discuss it with the Aries community? Any thoughts? |
Very sorry for the long delay in responding — lots of things are going on. 🧑🏭 I re-read the whole RFC (paying more attention this time…) and I’ve reversed my stance. It is pretty clear that the RFC says that the outcome is message-based, not protocol-based. To quote — So, to fully support I like your option 2 idea for now, but I would also be happy if we just implement the “On Receipt” part of the protocol. This is a bit of a rabbit hole, but as I’ve been thinking about this, I do see a possible issue with the intent of adding a ~please_ack on the This is a pretty complicated protocol... |
Signed-off-by: Alexander Sukhachev <[email protected]>
Hello @swcurran. Thank you for the answer.
Yes, it is how I have implemented my PoC code (option 2 in my message above).
Unfortunately, we have some unclear things related to the "common handler" (generic code to process the
Currently ACA-Py sends the ACK message after the presentation is verified. It happens automatically if the How can we continue this task? My idea is to start from something minimal that doesn't break backward compatibility between current and new versions of ACA-Py. And then to continue little-by-little.
Once it is done we can continue with the The biggest downside, in my opinion, is unconditional ACK. It means that I don't like the 'unconditional ACK' idea but we must not break backward compatibility. Alternative way is to postpone the What do you think? |
Thanks for the notes and suggestions. The unconditional Ack is messy. Even the “simple” Unfortunately, I don’t think we can progress right now — I just feel like the community is not ready for this. The impact of the protocol is too large, there are too many questions that have to be addressed about how it should be implemented, and it is not clear that there is a need for the feature. What I would like to do is have a discussion with production implementers to see where they would use the protocol if it were available. However, I think even that would be difficult — implementers probably wouldn’t think they needed it. The most important thing is how not to lose this information you have learned. Closing these PRs without put the learning some where would be a bad idea. How about I take on to do a presentation for the Aries Working Group in the next few weeks about what was found, to learn what others think, and to at minimum, put references into the Please Ack RFC to a summary of what was discovered in investigating this? Sorry for introducing this topic. What a mess! |
I've added a discussion section that I plan to go over at the next ACA-Pug meeting -- https://wiki.hyperledger.org/display/ARIES/2023-11-14+Aries+Cloud+Agent+-+Python+Users+Group+Community+Meeting. It is a bit of a dry-run for a similar discussion at an Aries Working Group meeting. |
Signed-off-by: Alexander Sukhachev <[email protected]>
Hello @swcurran
I have updated the document in this PR. I've tried to summarize important things and remove unnecessary according to our discussion here in comments. It would be excellent if you have a chance to review it. It is quite short, btw. I would ask you to think if this document (research, in fact) could be stored in the repo or not. If not, may be there is another place to store it. |
Signed-off-by: Alexander Sukhachev <[email protected]>
Kudos, SonarCloud Quality Gate passed! |
Hello @swcurran Could you please take a look at the final version of the document? |
We will not be moving forward with support for the please_ack decorator right now. Thanks for your efforts! |
To be more precise about the resolution for this PR, why it is being closed and to show how effort is linked to further explanations. The Aries Community is removing Thanks again for the work, @alexsdsr. Even though it didn’t get merged we learned a lot from this and moved DIDComm forward. The biggest learning is to make sure that we don’t “throw in” cross-cutting or optional features that alter the protocols state machines. For example — as recently come up — throwing in an optional, extra “problem-report” in response to a flawed final message of a protocol breaks the state machine. Either the extra message MUST be made every time, or not ever made — not sometimes... |
This PR is an attempt to make a start point of discussion how to implement the
please_ack
decorator support in ACA-Py. The PR contains initial version of the document that aims to find the final solution. It contains some thoughts/ideas about possible implementations.