GSMA-CPAS / BWRP-common-adapter

The "Layer 2.5" with all Common functionality APIs
Apache License 2.0
2 stars 0 forks source link

Add blockchain transaction id to response #23

Closed sgerhardt-trilobyte closed 3 years ago

sgerhardt-trilobyte commented 3 years ago

Regarding issue https://github.com/GSMA-CPAS/BWRP-UI/issues/52 UI should show blockchain related information such as blockchain transaction id in contract history view. Add blockchain transaction id to:

Blockchain transaction id should also be in the response after creating contract or signature

zkong-gsma commented 3 years ago

technically not sensible.

  1. transaction_id is useless to the Client, if they do not have an API to inspect details of the Transaction from the Peer. (unless they involve a system admin to retrieve the transaction from the backend).

  2. "POST /private-documents" do not return transaction_id.

We do store the "transaction_id" for signature. "revealing" them is easy, only if it make sense. (just like referenceId)

*just saw the latest version of update now includes "contract, storageKey, hash, referenceID. can we at least have a discussion about this addition before making the changes?

sgerhardt-trilobyte commented 3 years ago

Sorry, but what do you mean with "just saw the latest version of update now includes "contract, storageKey, hash, referenceID""?

zkong-gsma commented 3 years ago

can you at least discuss/mention or open an "issue" under block chain adapter to now include additional Responsese?

which got in during commit of

”implement proper errorcode forwarding“ https://github.com/GSMA-CPAS/BWRP-blockchain-adapter/commit/6868e00f9a8ce7f87ee589da80002fd6f0c9d470#diff-d130d137a468377dcdf0abbddddf29ade64be426de1a45024006d6b7e8529372

image image

as for instance, we developing common-adapter has no idea per-say that now we have and can store this information.

zkong-gsma commented 3 years ago

we will need to update "common-adapter" to store the "txID" now returned by blockchain-adapter before we can "reveal" it.

also after that. can we discusss what "key"/"name" should we expose this as.

bear in mind, from a design perspective, potentially in the future, when there is multi-ledger scenario, its better to be able to tell which belongs to which per-say. or a more specific name.

as txId is to "general"? as it can also means a "common-adapter" own "txId" or something else.

zkong-gsma commented 3 years ago

Based on the same principle from "https://www.w3.org/TR/did-core/#a-simple-example"

i would suggest something like.

"blockchain" : {"Identifier": "hlf", "id":"XXXXXXXXXX"}

where the same principle applies, meaning we keep a separate "master list" persay, eg

hlf => GSMA BWR eth => ethereum

and more importantly, behind these identifier, there should also be a "location" where the "network" is. simple idea is say if the identifier is "hlf", we know we are getting the information from our own "blockchain-adapter"

or like ethereum, it can be a public or private version, similar we then know which "network" to choose.

since its an "JSON" object, if required, we can expand to include some optional information, eg smart contract or anything else. but the base mandatory being "identifier" and "id"

zkong-gsma commented 3 years ago

as per requirement, we need to display the "txId" (per-say),

with the latest update.

the "submitter" that calls "POST /private_document" will receive the "txId" in the response.

however when the "recipient" receives the "webhook" from the blockchain-adapter, the "event" only has

payload := `{ ` +
        `"msp" : "` + invokingMSPID + `", ` +
        `"eventName" : "` + eventName + `", ` +
        `"timestamp" : "` + timestampString + `", ` +
        `"data" : { "storageKey" : "` + key + `" }` +
        ` }`

and "GET private_document" only provides.

// OffchainData struct
type OffchainData struct {
    FromMSP     string `json:"fromMSP"`
    ToMSP       string `json:"toMSP"`
    Data        string `json:"data"`
    DataHash    string `json:"dataHash"`
    TimeStamp   string `json:"timeStamp"`
    ReferenceID string `json:"id"`
    couchdb.Document
}

so, on the receiving end, we do not have any "txID" that we can store to reveal. any idea how we can get one?

sschulz-t commented 3 years ago

I added the txID to the event, please have a look at https://github.com/GSMA-CPAS/BWRP-blockchain-adapter/tree/ssch-txid-in-event

smeyerzu commented 3 years ago

i would suggest something like.

"blockchain" : {"Identifier": "hlf", "id":"XXXXXXXXXX"}

the base idea sound good to me. Nevertheless, I would suggest to modify it to the following:

"ledgerReference" : { "type": "hlf", "txID": "0x23232" }

Reasons:

zkong-gsma commented 3 years ago

i will argue otherwise. see example below image image

In my opinion, the word "ledger" is too "generic" and contains too generic meaning that reference to something else.

so, i will suggest not to use "ledger" per-say, but we need something else if we don't like "blockchain" either.

as for the object. I personally "accept" the "type" key, however for the "txId", similary, i felt that its again to a terms that is quite specific. where as some different technology uses different terms eg

txid, txn, txhash, transactionid, hash, tx

I think its better to have a "generic" terms that reference the lot, instead of using their specific names.

maybe something like. "XXXXXX" : { "type": "hlf", "reference": "0x23232" }

smeyerzu commented 3 years ago

In my opinion, the word "ledger" is too "generic" and contains too generic meaning that reference to something else.

Ok, so ledger might be too generic and blockchain too specific. I think the correct descriptor would be too long and cumbersome to read/say.

Let's come back to blockchain as for now with HLF and (maybe upcomiming) ETH it is correct. Still I would like to add Ref to make sure that this object is a reference to the "blockchain": blockchainRef

as for the object. I personally "accept" the "type" key

good!

however for the "txId", similary, i felt that its again to a terms that is quite specific. where as some different technology uses different terms eg

txid, txn, txhash, transactionid, hash, tx

I would say, all of those are "identifiers" for "transactions" on the shared ledger. So the short form txId is still a good term.

I think its better to have a "generic" terms that reference the lot, instead of using their specific names.

we just renamed the documentID to referenceID, using the same term reference here would cause a name-clash and thus cause confusion.

"blockchainRef" : { "type": "hlf", "txID": "0x23232" }

zkong-gsma commented 3 years ago

@smeyerzu while adding support for this, especially on the "receiving" end, I found some Issues.

Please provide some feedback/suggestions.

a "transaction id" is added to the "event" itself.

since the Design, (inherited from the web-ui),

upon receiving a Event, we are performing a "GET XXX/private-documents"

which returns a "list" of available "offchain-db" document.

And we then loop through them individually to further perform a "GET /private-documents/{referenceId}" to get an item.

theoretically, every time we received an "event", there should only be one "items" in the list. however there is a risk of "more", if we missed it or multiple submission happening at a same time.

when that happen, how/when can be have a "hard" association of the "txId" received with the "document"?

note: we can compare the "StoreageKey", but only 1 will match.

what do we do with the other one when StorageKey do not match?

Please provide some ideas?

sschulz-t commented 3 years ago

That's exactly what I was referring to in https://github.com/GSMA-CPAS/BWRP-common-adapter/issues/20 You will have to compare and match the storageKey to the local db.

For those that were missed in the past (network down or whatever happened) we might use a timer based function that collects those orphans and logs this so that we can investigate how those got missed in the first place?

zkong-gsma commented 3 years ago

i would had prefer the "txId" is added into the offchainDB automatically, however the chaincode flow design doesn't allow for it.

so that when we perform a "GET /private-documents/{referenceId}" we have the correct associated txid directly.

csarthou commented 3 years ago

I agree with Kong.

The returned object in a POST /private-documents should be the same as in a GET /private-documents/::referenceId

sschulz-t commented 3 years ago

we will investigate this issue and try to integrate a solution into the updated sequence diagrams and control flow that we are currently working on.

zkong-gsma commented 3 years ago

meanwhile, can you kindly commit the changes/PR of https://github.com/GSMA-CPAS/BWRP-common-adapter/issues/20 so that we(common-adapter) is able to test end2end please?

Thanks.

sschulz-t commented 3 years ago

meanwhile, can you kindly commit the changes/PR of #20 so that we(common-adapter) is able to test end2end please?

Which PR do you refer to? #20 links to the common adapter issue.

zkong-gsma commented 3 years ago

sorry. its the "branch" that you have produced the "quick" fix adding txId to the event.

we need it, and only then we can provide the "data" upstream to web-ui

sschulz-t commented 3 years ago

sorry. its the "branch" that you have produced the "quick" fix adding txId to the event.

https://github.com/GSMA-CPAS/BWRP-blockchain-adapter/pull/22 this one? I requested a review from you (maybe you have disabled notifications?). The idea is that you verify that it is working for you and then you trigger the merge by clicking the clicking the button. :)

sschulz-t commented 3 years ago

I agree with Kong.

The returned object in a POST /private-documents should be the same as in a GET /private-documents/::referenceId

This is finally implemented by https://github.com/GSMA-CPAS/BWRP-blockchain-adapter/pull/27

For example POST private-documents returns:

{
  "fromMSP": "ORG1",
  "toMSP": "ORG2",
  "payload": "ZGF0YTEyMzQ=",
  "payloadHash": "4508767822a67b7a051a8fe50250897c38c044ab10d9070d740a05a21deb4499",
  "blockchainRef": {
    "type": "hlf",
    "txID": "0xfe8948c4-ea59-4f21-b3c8-fddd18504e15",
    "timestamp": "2021-02-22T16:21:10+01:00"
  },
  "referenceID": "698c452e861d42a1948aa8c1de33584e34f8b55064d797762c2b71b2eb420c82"
}