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

Feature/ack credential #122

Merged
merged 11 commits into from
Oct 29, 2020
Merged

Feature/ack credential #122

merged 11 commits into from
Oct 29, 2020

Conversation

jakubkoci
Copy link
Contributor

This implements the ack part of the issue #64. I suggest closing that issue after the merge of this PR and eventually create other issues for the remaining functionality we would like to implement related to credential exchange.

What I see as remaining:

  • full implementation of message threading with attributes like sender_order and received_orders
  • alternative beginning with a credential proposal

@jakubkoci jakubkoci requested a review from a team as a code owner October 28, 2020 21:29
@jakubkoci
Copy link
Contributor Author

It should also fix #117 if I didn't forget something :)

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Awesome! I've added a lot of comments, but most are small, nitpicks or opinionated

src/lib/decorators/ack/AckDecoratorExtension.ts Outdated Show resolved Hide resolved
}

export class CredentialAckMessage extends AgentMessage {
public constructor(options: CredentialAckMessageOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if all values are optional, options should be optional. This way you wouldn't have to pass an empty object to the constructor.

Suggested change
public constructor(options: CredentialAckMessageOptions) {
public constructor(options?: CredentialAckMessageOptions) {

Copy link
Contributor Author

@jakubkoci jakubkoci Oct 29, 2020

Choose a reason for hiding this comment

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

That's a good point and I agree. However, if I do that we would need to change the logic to something like this:

    if (options) {
      this.id = options.id ?? this.generateId();
    } else {
      this.id = this.generateId();
    }

And I don't like that. I suggest to keep it for now and maybe change later if we see other cases like this one. Just to keep it consistent, you know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. We definitely don't want that.

Ideally we should get rid of the if (options) in all classes. There are others that have the same problem. class-transformer calls new XXX while it should use reflection directly. See the issue (and my comment). I'll try to take a look as it will be of huge benefit to us.

src/lib/decorators/ack/AckDecoratorExtension.ts Outdated Show resolved Hide resolved
src/lib/decorators/ack/AckDecorator.ts Outdated Show resolved Hide resolved
src/lib/decorators/ack/AckDecorator.test.ts Outdated Show resolved Hide resolved
@jakubkoci jakubkoci merged commit eeb703f into openwallet-foundation:master Oct 29, 2020
@jakubkoci jakubkoci deleted the feature/ack-credential branch October 29, 2020 17:11
ankita-p17 pushed a commit to ankita-p17/aries-framework-javascript that referenced this pull request Nov 6, 2020
* Add ack message and decorator
* Add credential ack handler
* Return credential record after processing credential offer message

Signed-off-by: Jakub Koci <[email protected]>
Signed-off-by: Ankita Patidar <[email protected]>
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