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

DRAFT: please_ack support PoC for the 0453-issue-credential-v2 protocol #2546

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions aries_cloudagent/config/argparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,14 @@ def add_arguments(self, parser: ArgumentParser):
"Default: false."
),
)
parser.add_argument(
"--cred-issue-ack-required",
action="store_true",
env_var="ACAPY_CRED_ISSUE_ACK_REQUIRED",
Copy link
Contributor

Choose a reason for hiding this comment

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

This raises another question -- how to define configuration settings for when to request ACKs? For example, is there a way with ArgParser to add a parameter type --ack-request-protocol <protocol> that can be defined multiple times? I don't know if that is even possible...

Likewise, should the Admin API be extended for supported protocols to add a add-please-ack on certain endpoints? That would mean that the controller would be responsible for using please ack when necessary/desirable.

Copy link
Author

Choose a reason for hiding this comment

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

I will check it

help=(
"An issuer sends please_ack to request a holder to send an ack"
"message in response to credential issuance"),
)

def get_settings(self, args: Namespace) -> dict:
"""Extract debug settings."""
Expand Down Expand Up @@ -468,6 +476,8 @@ def get_settings(self, args: Namespace) -> dict:
settings["debug.auto_accept_requests"] = True
if args.auto_respond_messages:
settings["debug.auto_respond_messages"] = True
if args.cred_issue_ack_required:
settings["debug.cred_issue_ack_required"] = True
return settings


Expand Down
29 changes: 28 additions & 1 deletion aries_cloudagent/messaging/agent_message.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import uuid
from collections import OrderedDict
from re import sub
from typing import Mapping, Optional, Text, Union
from typing import Mapping, Optional, Text, Union, Sequence

from marshmallow import (
EXCLUDE,
Expand All @@ -23,6 +23,7 @@
from .decorators.service_decorator import ServiceDecorator
from .decorators.signature_decorator import SignatureDecorator # TODO deprecated
from .decorators.thread_decorator import ThreadDecorator
from .decorators.please_ack_decorator import PleaseAckDecorator
from .decorators.trace_decorator import (
TRACE_LOG_TARGET,
TRACE_MESSAGE_TARGET,
Expand Down Expand Up @@ -283,6 +284,32 @@ def _service(self, val: Union[ServiceDecorator, dict]):
else:
self._decorators["service"] = val

@property
def _please_ack(self) -> PleaseAckDecorator:
"""Accessor for the message's please_ack decorator.

Returns:
The PleaseAckDecorator for this message
"""
return self._decorators.get("please_ack")

@_please_ack.setter
def _please_ack(self, val: Union[PleaseAckDecorator, dict]):
"""Setter for the message's please_ack decorator.

Args:
val: PleaseAckDecorator or dict to set as the please_ack
"""
if val is None:
self._decorators.pop("please_ack", None)
else:
self._decorators["please_ack"] = val

def assign_please_ack(self, on: Sequence[str]):
"""Assign a please_ack decorator with specified options
"""
self._please_ack = PleaseAckDecorator(on=on)

@property
def _thread(self) -> ThreadDecorator:
"""Accessor for the message's thread decorator.
Expand Down
2 changes: 2 additions & 0 deletions aries_cloudagent/messaging/decorators/default.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from .timing_decorator import TimingDecorator
from .transport_decorator import TransportDecorator
from .service_decorator import ServiceDecorator
from .please_ack_decorator import PleaseAckDecorator

DEFAULT_MODELS = {
"l10n": LocalizationDecorator,
Expand All @@ -18,6 +19,7 @@
"timing": TimingDecorator,
"transport": TransportDecorator,
"service": ServiceDecorator,
"please_ack": PleaseAckDecorator,
}


Expand Down
14 changes: 3 additions & 11 deletions aries_cloudagent/messaging/decorators/please_ack_decorator.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from marshmallow import EXCLUDE, fields

from ..models.base import BaseModel, BaseModelSchema
from ..valid import UUID4_EXAMPLE


class PleaseAckDecorator(BaseModel):
Expand All @@ -18,8 +17,7 @@ class Meta:

def __init__(
self,
message_id: str = None,
on: Sequence[str] = None,
on: Sequence[str]
):
"""Initialize a PleaseAckDecorator instance.

Expand All @@ -29,8 +27,7 @@ def __init__(

"""
super().__init__()
self.message_id = message_id
self.on = list(on) if on else None
self.on = list(on)


class PleaseAckDecoratorSchema(BaseModelSchema):
Expand All @@ -42,14 +39,9 @@ class Meta:
model_class = PleaseAckDecorator
unknown = EXCLUDE

message_id = fields.Str(
required=False,
allow_none=False,
metadata={"description": "Message identifier", "example": UUID4_EXAMPLE},
)
on = fields.List(
fields.Str(metadata={"example": "OUTCOME"}),
required=False,
required=True,
metadata={
"description": "List of tokens describing circumstances for acknowledgement"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,11 @@ async def handle(self, context: RequestContext, responder: BaseResponder):
)
)

cred_ack_message = await cred_manager.send_cred_ack(cred_ex_record)
if cred_ex_record.ack_required:
cred_ack_message = await cred_manager.send_cred_ack(cred_ex_record)
else:
cred_ack_message = await cred_manager.transit_to_done(cred_ex_record)


trace_event(
context.settings,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from ..manager import V20CredManager, V20CredManagerError
from ..messages.cred_problem_report import ProblemReportReason
from ..messages.cred_request import V20CredRequest
from ..models.cred_ex_record import V20CredExRecord


class V20CredRequestHandler(BaseHandler):
Expand Down Expand Up @@ -97,6 +98,10 @@ async def handle(self, context: RequestContext, responder: BaseResponder):
)
)

if cred_ex_record.state == V20CredExRecord.STATE_DONE and cred_ex_record.auto_remove:
self._logger.debug("delete cred_ex_record")
await cred_manager.delete_cred_ex_record(cred_ex_record.cred_ex_id)

trace_event(
context.settings,
cred_issue_message,
Expand Down
44 changes: 43 additions & 1 deletion aries_cloudagent/protocols/issue_credential/v2_0/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -515,8 +515,15 @@ async def issue_credential(
credentials_attach=[attach for (_, attach) in issue_formats],
)

cred_ex_record.state = V20CredExRecord.STATE_ISSUED
cred_ex_record.cred_issue = cred_issue_message

if self._profile.settings.get("debug.cred_issue_ack_required"):
# request the holder to answer with an ack message
cred_ex_record.state = V20CredExRecord.STATE_ISSUED
cred_issue_message.assign_please_ack(["OUTCOME"])
else:
cred_ex_record.state = V20CredExRecord.STATE_DONE

async with self._profile.session() as session:
# FIXME - re-fetch record to check state, apply transactional update
await cred_ex_record.save(session, reason="v2.0 issue credential")
Expand Down Expand Up @@ -550,6 +557,11 @@ async def receive_credential(
role=V20CredExRecord.ROLE_HOLDER,
)

please_ack = cred_issue_message._please_ack

if please_ack is not None and 'OUTCOME' in please_ack.on:
cred_ex_record.ack_required = True

cred_request_message = cred_ex_record.cred_request
req_formats = [
V20CredFormat.Format.get(fmt.format)
Expand Down Expand Up @@ -667,6 +679,36 @@ async def send_cred_ack(

return cred_ex_record, cred_ack_message


async def transit_to_done(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this "DONE" in the context of the entire protocol, or just in the context of the current message for which a "please-ack" OUTCOME was requested?

Or to put another way -- does each please-ack-aware protocol need to declare -- these are the things for which I am willing to ACK. For example, for issue one, I think:

  • If auto is set on, then no "RECEIPT" acks are except on "Issue" from the Issuer to the Holder (Holder responds)
  • If auto is not set on, the RECEIPT" acks whenever requested
  • OUTCOME Ack only from Holder to Issuer on completion of processing for the "Issue" message.

In other words -- only send Acks where there is not already a message going back to the other party at the same time.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Hello @swcurran. Thank you for your questions.

  1. This "DONE" is in the context of the protocol. Holder decides if it's time to send an 'ack' message or just to change state without sending it (it depends on whether an issuer has sent a 'please_ack' or not).

  2. I was thinking about your idea (decide to send an explicit 'ack' or not depending on whether there is expected messages in response to message containing a 'please_ack' or not). I see it is not mentioned in the please_ack RFC. There is only one sentence that implies that it's ok to ignore 'please_ack' in some cases:

agents also need the ability to request additional ACKs at other points in an instance of a protocol. Such requests may or may not be answered by the other party, hence the "please" in the name of decorator.

It sounds we can ignore the 'please_ack' when an agent is going to send response immediately. But first of all, it is only possible for 'RECEIPT', not for 'OUTCOME' (we discussed it here ). At the other hand, I think this behavior (sending explicit 'ack' message or not depending on some conditions) has to be defined in the 'please_ack' RFC.

My proposal is to continue implementation of support for 'OUTCOME'. And maybe to start discussion (in background with community) about possible behavior for agents who received 'RECEIPT' (is it ok to ignore in some cases or not? If yes, in what cases exactly? etc). Does it sound reasonable in your opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment on the Markdown file PR that I just posted. I’ve changed my mind about sending ACKs only sometimes — I think that does not make sense and is too hard to implement. My new thinking is that the use of ~please_ack should have no impact on the states, messages and processing of a protocol — it should just result in an extra message or two being sent at the appropriate time.

self,
cred_ex_record: V20CredExRecord,
):
"""Transition of the protocol instance to STATE_DONE

Delete cred ex record if set to auto-remove.

Returns:
cred ex record
"""

# FIXME - most of the code are copy-pasted from the send_cred_ack()
cred_ex_record.state = V20CredExRecord.STATE_DONE
try:
async with self._profile.session() as session:
await cred_ex_record.save(session, reason="store credential v2.0")

if cred_ex_record.auto_remove:
await self.delete_cred_ex_record(cred_ex_record.cred_ex_id)

except StorageError:
LOGGER.exception(
"Error transition to done"
)

return cred_ex_record


async def receive_credential_ack(
self, cred_ack_message: V20CredAck, connection_id: Optional[str]
) -> V20CredExRecord:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ def __init__(
auto_offer: bool = False,
auto_issue: bool = False,
auto_remove: bool = True,
ack_required: bool = False,
error_msg: str = None,
trace: bool = False, # backward compat: BaseRecord.from_storage()
cred_id_stored: str = None, # backward compat: BaseRecord.from_storage()
Expand All @@ -93,6 +94,7 @@ def __init__(
self.auto_offer = auto_offer
self.auto_issue = auto_issue
self.auto_remove = auto_remove
self.ack_required = ack_required
self.error_msg = error_msg

@property
Expand Down Expand Up @@ -222,6 +224,7 @@ def record_value(self) -> Mapping:
"auto_offer",
"auto_issue",
"auto_remove",
"ack_required",
"error_msg",
"trace",
)
Expand Down Expand Up @@ -425,6 +428,16 @@ class Meta:
"example": False,
},
)
ack_required = fields.Bool(
required=False,
dump_default=False,
metadata={
"description": (
"Issuer requests to send ack in response to credentials"
),
"example": False,
},
)
error_msg = fields.Str(
required=False,
metadata={"description": "Error message", "example": "The front fell off"},
Expand Down
14 changes: 9 additions & 5 deletions aries_cloudagent/protocols/issue_credential/v2_0/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -1559,11 +1559,15 @@ async def credential_exchange_store(request: web.BaseRequest):
# fetch these early, before potential removal
details = await _get_attached_credentials(profile, cred_ex_record)

# the record may be auto-removed here
(
cred_ex_record,
cred_ack_message,
) = await cred_manager.send_cred_ack(cred_ex_record)
if cred_ex_record.ack_required:
# the record may be auto-removed here
(
cred_ex_record,
cred_ack_message,
) = await cred_manager.send_cred_ack(cred_ex_record)
else:
cred_ex_record = await cred_manager.transit_to_done(cred_ex_record)
cred_ack_message = None

result = _format_result_with_details(cred_ex_record, details)

Expand Down
2 changes: 2 additions & 0 deletions demo/runners/support/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,8 @@ def get_agent_args(self):
("--endpoint", self.endpoint),
("--label", self.label),
"--auto-ping-connection",
# "--cred-issue-ack-required", # defines whether an issuer requires
# an explicit ack message from a holder
"--auto-respond-messages",
("--inbound-transport", "http", "0.0.0.0", str(self.http_port)),
("--outbound-transport", "http"),
Expand Down
Loading