fix: cannot set salts for credential creation (suggestion)

[lenkan]

This is my suggestion on how to solve #221 .

I know this is a breaking change, but I don't think we need the additional interface for IssueCredentialArgs. I think it just adds confusion with the additional mapping of properties. Same for the other operations such as create identifier, rotate etc.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.85%. Comparing base (e8b66e9) to head (49cd8d5).

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #222      +/-   ##
===============================================
- Coverage        82.86%   82.85%   -0.01%     
===============================================
  Files               47       47              
  Lines             4202     4200       -2     
  Branches          1047     1046       -1     
===============================================
- Hits              3482     3480       -2     
  Misses             691      691              
  Partials            29       29              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[psteniusubi]

It's good if the API can accept exn messages directly, without property mapping.

The API probably becomes harder to use with the compact one letter property names. Need to make sure all required properties are declared and have some reasonable documentation.

[seriouscoderone]

The readability of code is paramount.

We should fix this at the serialization level.

One option:
https://www.npmjs.com/package/typescript-json-serializer

@JsonObject()
export class Animal extends LivingBeing {
    @JsonProperty() name: string | undefined;
    @JsonProperty() birthDate: Date;
    @JsonProperty() numberOfPaws: number;
    @JsonProperty() gender: Gender;

    // Enum value (string)
    @JsonProperty() status: Status;

    // Specify the property name of json property if needed
    @JsonProperty('childrenIdentifiers')
    childrenIds: Array<number>;

    constructor(name: string | undefined) {
        super();
        this.name = name;
    }

}

[seriouscoderone]

This uses decorators on the objects. And leaves the code readable.

[lenkan]

In my opinion, decorators for serialization in JavaScript do not add any value. If we want to keep the API with more descriptive property names, I would vote for reverting back to plain property mapping. But my preference would be to move towards less mapping rather than more. I.e., using the same object shapes as KERIA accepts and returns.

[lenkan]

I just listened in on the dev call and wanted to provide some comments.

It seems like this change sparked some discussion on which direction to take regarding the API. I am voting for going towards a signify API that as closely as possible resembles the API of KERIA. The additional property mapping just is just another layer of indirection that in itself requires documentation. Some benefits of the approach:

• Documentation of the signify-ts API can be derived or implied from KERIA API documentation.
• Typescript types for request and response object shapes can probably be completely generated by openapi tooling (once Openapi specs for keria is complete).
• Less issues of the type No support for multisig issuance of private ACDCs #221 (there has previously been more issues like this)

Also, for this particular API of issuing credential, a user of this library could add their own functions for ACDC creation according to which ever schema they want to use, like this.

function createQviCredentialData(args: QviCredentialData): CredentialData {
    return {
        ri: args.registryId,
        i: args.issuerPrefix,
        s: "EBf...",
        a: { LEI: args.LEI, i: args.recipientPrefix }
    }
}

const data = createQviCredentialData({ registryId, issuerPrefix: "abc", recipientPrefix: "def", LEI: "123" })
await credentials().issue("issuer", data)

As @psteniusubi mentioned as well, the fact that the API can accept the same shape of objects that is passed in exchange messages is a huge plus in my opinion, as can be seen in this change: 7022f95#diff-6d0716ef2bd3e3e9494efe4b175ef940675ac2c0615c4d98172c7c5f69671c9aR906.

If we decide to go for this approach, we should probably plan to change the rest of the API to do follow the same strategy and then strictly adhere to it going forward.

I do see the argument against it as well though, using more descriptive property names is helpful, but the benefits mentioned are just much greater in my opinion.

[psteniusubi]

@lenkan - I agree with the suggestion to move forward with compact labels.

The Keri spec already has definitions for message types and labels. (I could not immediately find all message types, for example "iss" and "vcp" are missing.)

As you suggest it is relatively easy to build a higher lever abstraction on top of the lower level Signify API. One could even implement a backwards compatibility layer.

There is at least one catch. Because Keri sets strict rules on ordering of the fields, the Signify API should still internally re-build all messages. I don't think the API can internally use the spread ... operator. For messages that originate from the API client ordering is not known, or there could exist unknown fields.

Should this design change apply to signifypy as well?