GSMA-CPAS / BWRP-chaincode

Apache License 2.0
1 stars 0 forks source link

Use payload link key for storage only #54

Closed informartin closed 3 years ago

informartin commented 3 years ago

So far, the composite key owner~type~key~txid was used for signatures as well as payload links. This comes with multiple drawbacks:

  1. As the composite key includes an owner, external knowledge is required about the creator of the payload link.
  2. Conceptually, there is no single owner of a contract, as contracts are always shared.
  3. Because a payload link should always only be stored once, using the transaction id as part of the composite key requires further checks if multiple payload links have been stored.

Therefore, I would propose to store the payload link using the key only, removing the composite key. In this case, we must ensure that the value cannot be overwritten though. Because the key is the hash of the referenceId, there shouldn't be any collisions.

informartin commented 3 years ago

Thanks, Simon, for reviewing and summarizing the most important aspects of this PR.

If I'm not overlooking anything, the signature storage location should not be affected by this PR though. As multiple signatures are stored for a single documents, we still need the composite key including txId, I think. I'm not quite sure if removing the owner field from the signature composite key comes any benefits or drawbacks, do you have any thoughts on this?

You are right that we cannot easily retrieve the sender anymore, I cannot think of any situation where this would be needed though. We also do not offer that functionality yet, do we? As you said, in case of legal disputes, the transaction can be used to retrieve that information.

Concerning the "reservation" issue, I'm quite positive that this won't be an issue, as reserving large portions of a 256 bit space will arouse suspicion of other participants and be infeasible in practice.

sschulz-t commented 3 years ago

If I'm not overlooking anything, the signature storage location should not be affected by this PR though.

Sorry my mistake, i mixed up the signature and payloadlink call, I corrected the comment above ;)

sschulz-t commented 3 years ago

I am working on a PR for the blockchainadapter to support this chaincode PR.

func (s *RoamingSmartContract) IsValidSignature(ctx contractapi.TransactionContextInterface, creatorMSPID, signaturePayload, signature, signatureAlgorithm, certChainPEM string) error {

I think we should rename creatorMSPID in IsValidSignature to signerMSPID in order to be more clear what this id is about. Even though this has nothing to do with this pr (the naming was wrong before) maybe we should fix it here ;)

informartin commented 3 years ago

Good find, you are right, that would make more sense, I will update it accordingly.