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

Add a quickstart library file #25

Merged
merged 17 commits into from
Mar 30, 2024
Merged

Add a quickstart library file #25

merged 17 commits into from
Mar 30, 2024

Conversation

TheTechmage
Copy link
Contributor

Adds a quickstart.py file which is intended to provide a "quick-start" for those intending to get started with this library, but may not want to know all of the nitty-gritty details quite yet. This is intended to support https://github.com/decentralized-identity/didcomm-book, making it even easier for newcomers to get started with DIDComm.

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
@TheTechmage TheTechmage self-assigned this Jan 18, 2024
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
@TheTechmage TheTechmage marked this pull request as ready for review January 18, 2024 13:54
Copy link
Member

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

I feel like I might have been a bit stricter in my comments than I would normally be -- sorry! But I hope these comments will help the quickstart be as easy to pick up as possible for new users.

didcomm_messaging/quickstart.py Outdated Show resolved Hide resolved
didcomm_messaging/quickstart.py Outdated Show resolved Hide resolved
didcomm_messaging/quickstart.py Outdated Show resolved Hide resolved
didcomm_messaging/quickstart.py Outdated Show resolved Hide resolved
didcomm_messaging/quickstart.py Outdated Show resolved Hide resolved
Comment on lines 332 to 337
async def fetch_relayed_messages(
dmp: DIDCommMessaging,
my_did: DID,
relay_did: DID,
callback: Callable[[Message], Awaitable[None]] = None,
) -> List[Message]:
Copy link
Member

Choose a reason for hiding this comment

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

Subtle issue, our mediator will always respond as though return route had been set on the inbound message. However, this is not strictly correct behavior. These messages should include return route. And we should probably make assertions about the inclusion of return route in the mediator's handlers.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added "return-route": "all" to all messages by default to the quickstart

didcomm_messaging/quickstart.py Outdated Show resolved Hide resolved
didcomm_messaging/quickstart.py Outdated Show resolved Hide resolved
didcomm_messaging/quickstart.py Outdated Show resolved Hide resolved
Comment on lines +282 to +290
[
{
"type": "DIDCommMessaging",
"serviceEndpoint": {
"uri": relay_did,
"accept": ["didcomm/v2"],
},
}
],
Copy link
Member

Choose a reason for hiding this comment

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

Most of this is static; should we just have a parameter that takes the relay_did and makes the rest of that in the generate method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have the generate method in did:peer:N allow either a DID or a service structure, I could agree. However, showing the structure here does also provide the benefit of teaching people that "this is what a basic service block looks like when you generate a DID"

Copy link
Member

Choose a reason for hiding this comment

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

I think we get that "this is what it looks like" by having the generate method nearby in the quickstart module (which it is).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the generate method is defined elsewhere, I have created two issues regarding this change:

The solution should be handled in their respective projects.

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
@TheTechmage TheTechmage requested a review from dbluhm March 22, 2024 15:14
@dbluhm dbluhm merged commit ccc9b77 into main Mar 30, 2024
5 checks passed
@TheTechmage TheTechmage deleted the minimal branch April 1, 2024 16:50
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.

2 participants