blockchain-certificates / cert-issuer

Issues Blockcerts using either the Bitcoin or Ethereum blockchain
MIT License
414 stars 208 forks source link

Missing API to issue credentials into the Ethereum Blockchain #290

Closed abhisheksarka closed 3 months ago

abhisheksarka commented 3 months ago

Currently there is only one API to issue credentials into the Bitcoin blockchain. It would be great to introduce the same for Ethereum as well.

Proposed Changes: Introduce a new API called /cert_issuer/api/v1.0/eth_issue. This would keep the /cert_issuer/api/v1.0/issue API unchanged.

PR https://github.com/blockchain-certificates/cert-issuer/pull/289

lemoustachiste commented 3 months ago

So I have not designed nor used the API issuance of Cert Issuer so I'm not familiar with the internals here.

But as I suppose this would be a POST request, can't it just be part of the request payload, as a flag?

abhisheksarka commented 3 months ago

The current API which is POST /cert_issuer/api/v1.0/issue accepts a payload that is an array of certs.

Payload:

[
  { ...cert1 },
  { ...cert2 },
  { ...certn },
]

So, considering this structure if we want to accommodate the flag in the same API, it would have to be a url param POST /cert_issuer/api/v1.0/issue?type=ethereum

This would be backwards compatible so could work. However, to keep things separate I added the new API. I think having a single API works too with the type params. What do you suggest?

lemoustachiste commented 3 months ago

Ok,

I think then a better approach would be to keep the API path structure:

/cert_issuer/api/v1.0/issue/ethereum and with /cert_issuer/api/v1.0/issue still working (and defaulting to bitcoin to maintain backwards compatibility) and maybe aliasing for /cert_issuer/api/v1.0/issue/bitcoin to add some consistency and better devX for calling code (the chain can be held into a variable this way).

Does that sound reasonable?

abhisheksarka commented 3 months ago

Yes, this is definitely cleaner than having the type parameter. I'll update the attached PR.