refactor: improve vc export api surface

[rflechtner]

What changes:

• makes the timestamp an optional parameter on KiltCredentialV1.fromInput and fixes a bug on this function.
• also changes the type of its ctype parameter to be a CType id only, as the CType object has no use here any more.
• Improves the typing of a credential object prior to attestation, where a number of fields may be omitted as they are overwritten anyway.
• changes KiltAttestationV1Suite.createProof so that it returns a complete proof object. There is still another step needed prior to calling this (anchorCredential) which produces the attestation and adds it to chain, as well as edits the credential so that it becomes verifiable with this proof (id, issuanceDate, credentialStatus, etc).
• Removes the api parameter of the attestation suite - simply uses the cached api now, as this did not filter down to all functions called under the hood.
• groups the did & didSigner parameters on issue to a didSigner interface and the submitterAddress & txSubmissionHandler to a transactionHandler interface. Callers now also have the choice between implementing submission themselves or only supplying a transaction signer, in which case a default submitter callback will be constructed.

How to test:

Ideally, try out the new api surface and see if it is nice(r) to use.

Checklist:

• I have verified that the code works
• I have verified that the code is easy to understand
  • If not, I have left a well-balanced amount of inline comments
• I have left the code in a better state
• I have documented the changes (where applicable)
  • Either PR or Ticket to update the Docs
  • Link the PR/Ticket here

[ntn-x2]

Would it make sense to expose one additional method that returns an object that the createProof proof then takes? So instead of exposing a createProof and saying that the finalizeProof must be called, having a method that returns a NonFinalizedProof, which is then passed to the createProof to finalize the object? So basically the first method does what the createProof does now, and createProof would do what finalizeProof does now.

In short, relying on the static type system TS provides to make sure that a user won't be able to use a half-created proof.

[rflechtner]

If you look at the implementation in jsonld-signatures, this is called only with the following arguments:

    const proof = await suite.createProof({
      document: input, purpose, documentLoader
    });

-> there is no good way to pass additional input besides the document to be signed, which also cannot be altered in the signing process. I'm saying no good way bc you could probably craft a purpose that contains the non-finalised proof, but then again, where is the point if the result is a document that still can't be verified with that proof?

The changes I proposed make the suite class stateless. If you want to swap the order of things, I could offer that we keep it stateful and require users to call some method (maybe anchorCredential) before using createProof or functions that rely on it (jsigs sign & vc-js issue). In that case, the story would be 'you first anchor the credential on the blockchain, then you create a proof over it being anchored'. Note that in this case, anchorCredential would be the step at which an attestation is made, and no longer createProof. The class instance would then keep track of data required to create the proof (block of inclusion, nonces, etc) and look them up on creating a proof.

[ntn-x2]

I think it would be best, if not possible to have compilation-time errors, to have runtime errors before the user sends anything to the verifier. Hence, we should aim to provide a guarantee that "if the proof has been generated and sent following the logic in this SDK, it won't fail to pass the verifier's checks (except revocation etc etc)".
So it's probably fine to have one more step in between, rather than entirely relying on documentation, as is the case now, AFAICT.

[rflechtner]

a pretty easy fix for producing compile-time errors if you try to send off a credential that has not yet been run through finalizeProof would be creating proofs where the type field is something else (e.g. NonFinalizedKiltAttestationProof), which is then changed once processed. Personally, I don't really like that solution though... It seems like a more straightforward interface to require preprocessing/anchoring first, and then create the proof after the fact. If you get past this point you are sure to have a valid credential.

[ntn-x2]

I like it! I am just not sure about specifying the TxHandler so early on in the process of creating a new credential.

[ntn-x2]

I see the advantage of having a DidSigner as a class member, but what if the TxHandler would be provided only when needed?

[rflechtner]

Both are optional, but it’s true that they are only used by a single method, so if you think they should be parameters on it instead, that can be done

[ntn-x2]

Both are optional, but it’s true that they are only used by a single method, so if you think they should be parameters on it instead, that can be done.

I think moving the TxHandler to the time it is used would be enough. Move also the DidSigner along with it if you think that having one passed in the constructor and one passed at the time of the operation is too confusing, otherwise I would be fine with splitting them like that.

[rflechtner]

Both are optional, but it’s true that they are only used by a single method, so if you think they should be parameters on it instead, that can be done.

I think moving the TxHandler to the time it is used would be enough. Move also the DidSigner along with it if you think that having one passed in the constructor and one passed at the time of the operation is too confusing, otherwise I would be fine with splitting them like that.

In my eyes, any argument that applies to the txHandler applies to the didSigner just as much, so I'd move them both.