Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Asset-Transfer-Events Migration to Use Fabric-Gateway #565

Merged
merged 3 commits into from Feb 2, 2022

Conversation

sapthasurendran
Copy link
Contributor

No description provided.

@sapthasurendran sapthasurendran requested a review from a team as a code owner December 16, 2021 18:02
'CreateAsset',
{ arguments:[assetId, 'blue', '10', 'James', '100'],
transientData: asset_properties,
endorsingOrganizations: [mspId]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to specify the endorsing orgs in this case. The gateway will select its own org by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the endorsement orgs,I got 'channel shut down' error

const commit = await contract.submitAsync('TransferAsset', {
arguments: [assetId, 'David'],
transientData: asset_properties,
endorsingOrganizations: [mspId]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I don't think endorsingOrgs is needed.

Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things to tweak in the code to demonstrate better practices in event handling and remove clutter that I think distract from that event handling -- see comments in the code.

In addition:

  1. I'm not certain why the flow does a similar set of work with both regular assets and then again with private data, given it's trying to demonstrate eventing and we have a separate private data sample. @denyeart might know. If it's not necessary I would remove it.
  2. There is a lot of console output from the transaction invocations that make it hard to see the console output from the events. I think I would strip the transaction output right down to make it easier to see in the console the sequence of transaction invocations and events received. Particularly as with this async event listening the transaction submissions and events received are often interleaved in different ways.
  3. The code only demonstrates real-time event listening. If we cut out all the other junk, perhaps it would be worth also demonstrating event replay by keeping track of the first block number where we receive real-time events and, at the end of the transaction flow, replaying events from that block number? Again, @denyeart may have an opinion.
  4. The sample doesn't deal with reconnecting if the event stream disconnects. Realistically, that's not going to happen when running the sample, and doing it properly to avoid missing or receiving duplicate events may be too involved for a starter sample. This might be one to wait for when there is some out-of-the-box support for this in the Fabric Gateway API (likely using checkpointers).

Comment on lines 7 to 31
// pre-requisites:
// - fabric-sample two organization test-network setup with two peers, ordering service,
// and 2 certificate authorities
// ===> from directory test-network
// ./network.sh up createChannel -ca
//
// - Use the asset-transfer-events/chaincode-javascript chaincode deployed on
// the channel "mychannel". The following deploy command will package, install,
// approve, and commit the javascript chaincode, all the actions it takes
// to deploy a chaincode to a channel.
// ===> from directory test-network
// ./network.sh deployCC -ccn events -ccp ../asset-transfer-events/chaincode-javascript/ -ccl javascript -ccep "OR('Org1MSP.peer','Org2MSP.peer')"
//
// - Be sure that node.js is installed
// ===> from directory asset-transfer-events/application-javascript
// node -v
// - npm installed code dependencies
// ===> from directory asset-transfer-events/application-javascript
// npm install
// - to build this test application
// ===> from directory asset-transfer-events/application-javascript
// npm prepare
// - to run this test application
// ===> from directory asset-transfer-events/application-javascript
// npm start
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very keen on all this README type content cluttering up the file. It might be better in the README.md, which already contains similar (but wrong!) information. Maybe remove these comments and fix the README?

const peerEndpoint = 'localhost:7051';

const utf8Decoder = new TextDecoder();
let assetId = `asset${Date.now()}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module scope variable not being const, getting modified part way through the application flow, and being referenced by several sub-methods seems like a bit of a red flag. I wonder if the code should be structured differently or, if you just need two different asset IDs, just create two up-front with something like:

const now = Date.now();
const sampleAssetId = `asset${now}`;
const privateAssetId = `asset${now + 1}`;

Comment on lines 141 to 108
async function startEventListening(network: Network) {
console.log('Read chaincode events');
const events = await network.getChaincodeEvents(chaincodeName);
try {
for await (const event of events) {
const payload = utf8Decoder.decode(event.payload);
console.log(`Received event name: ${event.eventName}, payload: ${payload}, txID: ${event.transactionId}, blockNumber:${event.blockNumber}`);
}
} finally {
// Ensure event iterator is closed when done reading.
events.close();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is problematic as the application just hangs at the end of the main body because this event reading loop is still running. events.close() should probably get called to close the event stream at the end of the main application body. Unfortunately that will mean this for-await loop will terminate with an error because the stream it's reading from is cancelled.

There are a few options. If you want to keep this simple (non-terminating) event reading loop then you might have to:

  1. Return the events variable to the main function so it can call close() on it at the end of execution.
  2. Handle any exception thrown here gracefully.

To close the events correctly, you could change the main() function to include something like:

let events: CloseableAsyncIterable<ChaincodeEvent> | undefined;
try {
    events = await startEventListening(network);
} finally {
    events?.close();
}

To handle an error terminating the for-await loop you might check the gRPC status code to see if it was cancelled (by your call to events.close()), something like:

async function startEventListening(network: Network): Promise<CloseableAsyncIterable<ChaincodeEvent>> {
    console.log('Read chaincode events');
    const events = await network.getChaincodeEvents(chaincodeName);
    readEvents(events); // Don't await -- let this continue in the background
    return events;
}

async function readEvents(events: CloseableAsyncIterable<ChaincodeEvent>): Promise<void> {
    try {
        for await (const event of events) {
            const payload = utf8Decoder.decode(event.payload);
            console.log(`Received event name: ${event.eventName}, payload: ${payload}, txID: ${event.transactionId}, blockNumber:${event.blockNumber}`);
        }
    } catch (error: unknown) {
        if (!(error instanceof GatewayError) || error.code !== grpc.status.CANCELLED) {
            throw error;
        }
    }
}

Another alternative might be to get an AsyncIterator from the AsyncIterable with const iterator = events[Symbol.asyncIterator](); and then read from the iterator using await iterator.next() after submitting a transaction to receive the event you expect to be emitted. The syntax is not very pretty though.

Similar to using the iterator directly as I just described, but without having to dig into the AsyncIterator API, you could have a helper function to read a single event that uses the for-await syntax. Something like:

async function readEvent(events: CloseableAsyncIterable<ChaincodeEvent>): Promise<void> {
    for await (const event of events) {
        const payload = utf8Decoder.decode(event.payload);
        console.log(`Received event name: ${event.eventName}, payload: ${payload}, txID: ${event.transactionId}, blockNumber:${event.blockNumber}`);
        break;
    }
}

Comment on lines 223 to 199
/**
* submitTransaction() will throw an error containing details of any error responses from the smart contract.
*/
async function updateNonExistentAsset(contract: Contract): Promise<void>{
console.log('\n--> Submit Transaction: UpdateAsset asset70, asset70 does not exist and should return an error');

try {
await contract.submitTransaction(
'UpdateAsset',
'asset70',
'blue',
'5',
'Tomoko',
'300',
);
console.log('******** FAILED to return an error');
} catch (error) {
console.log('*** Successfully caught the error: \n', error);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just repeats something already demonstrated in the basic sample. It is nothing to do with events and just adds clutter, so I would remove it.

Comment on lines 244 to 150
/**
* Verify asset details.
*/
function checkAsset(mspId:string, asset:{
ID: string,
Color: string,
Size: string,
Owner: string,
AppraisedValue: string,
asset_properties:{
object_type: string,
asset_id: string,
Price: string,
salt: string
}
}, color:string, size:string, owner:string, appraisedValue:string, price?:string) {
console.log(`<-- Query results from ${mspId}`);

console.log(`*** verify asset ${asset.ID}`);

if (asset) {
if (asset.Color === color) {
console.log(`*** asset ${asset.ID} has color ${asset.Color}`);
} else {
console.log(`*** asset ${asset.ID} has color of ${asset.Color}`);
}
if (asset.Size === size) {
console.log(`*** asset ${asset.ID} has size ${asset.Size}`);
} else {
console.log(`*** Failed size check from ${mspId} - asset ${asset.ID} has size of ${asset.Size}`);
}
if (asset.Owner === owner) {
console.log(`*** asset ${asset.ID} owned by ${asset.Owner}`);
} else {
console.log(`*** Failed owner check from ${mspId} - asset ${asset.ID} owned by ${asset.Owner}`);
}
if (asset.AppraisedValue === appraisedValue) {
console.log(`*** asset ${asset.ID} has appraised value ${asset.AppraisedValue}`);
} else {
console.log(`*** Failed appraised value check from ${mspId} - asset ${asset.ID} has appraised value of ${asset.AppraisedValue}`);
}
if (price) {
if (asset.asset_properties && asset.asset_properties.Price === price) {
console.log(`*** asset ${asset.ID} has price ${asset.asset_properties.Price}`);
} else {
console.log(`*** Failed price check from ${mspId} - asset ${asset.ID} has price of ${asset.asset_properties.Price}`);
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking more like a roll-your-own unit test and adds a huge amount of distracting clutter to the sample. It's nothing to do with eventing so I would remove it.

@sapthasurendran
Copy link
Contributor Author

A few things to tweak in the code to demonstrate better practices in event handling and remove clutter that I think distract from that event handling -- see comments in the code.

In addition:

  1. I'm not certain why the flow does a similar set of work with both regular assets and then again with private data, given it's trying to demonstrate eventing and we have a separate private data sample. @denyeart might know. If it's not necessary I would remove it.
  2. There is a lot of console output from the transaction invocations that make it hard to see the console output from the events. I think I would strip the transaction output right down to make it easier to see in the console the sequence of transaction invocations and events received. Particularly as with this async event listening the transaction submissions and events received are often interleaved in different ways.
  3. The code only demonstrates real-time event listening. If we cut out all the other junk, perhaps it would be worth also demonstrating event replay by keeping track of the first block number where we receive real-time events and, at the end of the transaction flow, replaying events from that block number? Again, @denyeart may have an opinion.
  4. The sample doesn't deal with reconnecting if the event stream disconnects. Realistically, that's not going to happen when running the sample, and doing it properly to avoid missing or receiving duplicate events may be too involved for a starter sample. This might be one to wait for when there is some out-of-the-box support for this in the Fabric Gateway API (likely using checkpointers).

@denyeart Can you please confirm the changes ?

@denyeart
Copy link
Contributor

denyeart commented Jan 3, 2022

1 - The code for processing events is different for public data versus private data. Since this is the main event sample, I think it should continue to demonstrate both public data and private data. The 'base' public and private samples intentionally don't demonstrate eventing to keep them very simple for newbies.

3 - The https://github.com/hyperledger/fabric-samples/tree/main/off_chain_data sample demonstrates how to do event checkpointing based on last processed block, but it is for block events. So I'll send the question back to @bestbeforetoday - could we just reference the off_chain_data sample for this pattern or is it different enough to justify adding the sample code in this one?

@bestbeforetoday
Copy link
Member

1 - The code for processing events is different for public data versus private data. Since this is the main event sample, I think it should continue to demonstrate both public data and private data.

This sample is currently only demonstrating chaincode event listening, since that is the only type of event listening currently provided by the Fabric Gateway client API. There is no difference in eventing behaviour for transaction functions using public or private data. The only difference we are demonstrating is setting transient data on the transaction invocation -- nothing to do with eventing.

For demonstrating chaincode eventing using the Fabric Gateway client API, we should remove the private data variants for clarity.

3 - The https://github.com/hyperledger/fabric-samples/tree/main/off_chain_data sample demonstrates how to do event checkpointing based on last processed block, but it is for block events. So I'll send the question back to @bestbeforetoday - could we just reference the off_chain_data sample for this pattern or is it different enough to justify adding the sample code in this one?

The checkpointing aspect of the off_chain_data sample just duplicates capability already provided out-of-the-box by the SDK's checkpointer capability, so I'm not convinced it a good usage example anyway.

The off_chain_data sample doesn't deal with reconnecting the event stream in the case that connection to the peer is lost. The Node SDK does this transparently to the client application using the internal checkpointing logic. The current Fabric Gateway client API implementation exposes disconnections (from the Gateway) to the client application and they would need to reconnect programmatically. This relies on the application storing the last processed block number and reconnecting with a specified start block. This is something I'm expecting to be easier (or perhaps handled transparently) if/when we get checkpointing implemented in the Fabric Gateway client API.

@sapthasurendran sapthasurendran force-pushed the asset-events branch 2 times, most recently from e3904c3 to e8f0f03 Compare January 12, 2022 11:44
@sapthasurendran
Copy link
Contributor Author

@bestbeforetoday Could you please do a review ? . Let me know if there is any additional information to be added in theREADME to better describe the event handling used in gateway.

Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running the application I get this output:

% npm start

> asset-transfer-basic@1.0.0 start
> node dist/app.js

Read chaincode events

--> Submit Transaction: CreateAsset, creates new asset with ID, Color, Size, Owner and AppraisedValue arguments

--> Async Submit Transaction: TransferAsset, updates existing asset owner
Received event name: CreateAsset, payload: {"ID":"asset1642096463648","Color":"yellow","Size":"5","Owner":"Tom","AppraisedValue":"1300"}, txID: 9ada11a07770264a818df0d8999b6afce113a555b9fb66901265143eca27690c, blockNumber:6
*** Successfully submitted transaction to transfer ownership from {"type":"Buffer","data":[]} to Saptha
*** Waiting for transaction commit
Received event name: TransferAsset, payload: {"ID":"asset1642096463648","Color":"yellow","Size":"5","Owner":"Saptha","AppraisedValue":"1300"}, txID: fbc23dc219a199591e23b07bdf30504ab40f9c089a90d2de94037eac3d50404f, blockNumber:7

--> Submit Transaction: DeleteAsset asset70

The application terminates before the event from the DeleteAsset call is received and printed. Maybe this is OK but I think it might be clearer if the application waits until the delete event is received. You could create a module-scope Promise variable that you await on before completing the main application flow. You resolve this promise when the last event is received, so it acts as a barrier preventing the application from exiting until the expected events are received.

Comment on lines 95 to 96
# ensure this line in app.js have correct chaincode deploy name
# const chaincodeName = '...';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just running the commands in this README as-is results in a failure running the client application since the chaincode name doesn't match the on used at deployment. I think the sample should work out-of-the-box and not require the user to modify the sample code before it will work.

I would suggest changing the ./network.sh deployCC command above so specify the same chaincode name as the sample code:

./network.sh deployCC -ccs 1 -ccv 1 -ccep "OR('Org1MSP.peer','Org2MSP.peer')" -ccl java -ccp ./../asset-transfer-events/chaincode-java/ -ccn events

- smart contract calls (console output like `--> Submit Transaction or --> Evaluate`)
- the events received at application end (console output like `<-- Contract Event Received: or <-- Block Event Received`)
- the events received at application end (console output like `<-- Contract Event Received: or <-- Block Event Received`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might read better with the two types of message separated:

`<-- Contract Event Received:` or `<-- Block Event Received`

* Submit a transaction synchronously to delete an asset by ID.
*/
async function deleteAssetByID(contract:Contract): Promise<void>{
console.log('\n--> Submit Transaction: DeleteAsset asset70');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asset ID is not asset70

try {
for await (const event of events) {
const payload = utf8Decoder.decode(event.payload);
console.log(`Received event name: ${event.eventName}, payload: ${payload}, txID: ${event.transactionId}, blockNumber:${event.blockNumber}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The README says event receipt messages will begin with <--, which mirrors the transaction submit messages that begin with -->. This message should change to match that pattern.

the events received at application end (console output like <-- Contract Event Received: or <-- Block Event Received)

Comment on lines 138 to 150
async function transferAssetAsync(contract: Contract): Promise<void> {
console.log('\n--> Async Submit Transaction: TransferAsset, updates existing asset owner');

const commit = await contract.submitAsync('TransferAsset', {
arguments: [assetId, 'Saptha'],
});
const oldOwner = utf8Decoder.decode(commit.getResult());

console.log(`*** Successfully submitted transaction to transfer ownership from ${oldOwner} to Saptha`);
console.log('*** Waiting for transaction commit');

const status = await commit.getStatus();
if (!status.successful) {
throw new Error(`Transaction ${status.transactionId} failed to commit with status code ${status.code}`);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The basic sample already demonstrates async submit. I don't think the async calling pattern affects the eventing (which is what this sample is demonstrating) so I would keep it simple and just use the one-line submitTransaction() call style.

} finally {
gateway.close();
client.close();
events?.close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

events should close before gateway (and have a ; at the end of the line). The eventing is carried out within the scope of a given gateway, so it makes sense to close resources before closing their containing resource.

client is the underlying gRPC connection that gateway (and events) use. It should be closed last after everything that's using it is closed.

@bestbeforetoday
Copy link
Member

@sapthasurendran Sorry to disrupt the fine work you've put into this sample already! I have re-worked the asset-transfer-events sample application (in Go to avoid conflicting with what you've done in TypeScript) to provide a better demonstration of the chaincode event listening capability in the Fabric Gateway client API:

Note that I have separated the connection code (which is just boiler-plate that duplicates what is already demonstrated in the basic sample) from the main application code to make it easier to read. I think we just need the TypeScript sample to:

  1. Behave the same as the Go one.
  2. Output the same console messages.

The README will only need the command-line for launching the TypeScript version of the application added. The rest of the instructions should work as-is.

Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
@sapthasurendran sapthasurendran force-pushed the asset-events branch 2 times, most recently from 3a56476 to 29b0fa8 Compare February 1, 2022 12:01
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of comments but all minor things. Generally looks great!

const channelName = 'mychannel';
const chaincodeName = 'events';

// Path to crypto materials.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment isn't attached to any code in this file so could be removed.

Comment on lines 95 to 98
cd application-gateway-typescript
npm install
npm run prepare
npm start
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prepare script is run as part of the npm install

Suggested change
cd application-gateway-typescript
npm install
npm run prepare
npm start
cd application-gateway-typescript
npm install
npm start

main().catch(error => console.error('******** FAILED to run the application:', error));


async function startEventListening(network: Network) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async function startEventListening(network: Network) {
async function startEventListening(network: Network): Promise<CloseableAsyncIterable<ChaincodeEvent>> {

}
}

main().catch(error => console.error('******** FAILED to run the application:', error));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkneubuh spotted in the basic sample that we didn't correctly set the exit code to non-zero on error.

Suggested change
main().catch(error => console.error('******** FAILED to run the application:', error));
main().catch(error => {
console.error('******** FAILED to run the application:', error);
process.exitCode = 1;
});

https://nodejs.org/api/process.html#processexitcode_1

Comment on lines 83 to 91
console.log('\n*** Start chaincode event listening\n');
const events = await network.getChaincodeEvents(chaincodeName);
readEvents(events);
return events;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally wouldn't worry about this but since it's a sample some whitespace might help the reader spot the relevant client API call amongst the application code:

Suggested change
console.log('\n*** Start chaincode event listening\n');
const events = await network.getChaincodeEvents(chaincodeName);
readEvents(events);
return events;
console.log('\n*** Start chaincode event listening\n');
const events = await network.getChaincodeEvents(chaincodeName);
readEvents(events);
return events;

Comment on lines 108 to 117
const result = await contract.submitAsync(
'CreateAsset',
{
arguments: [
assetId,
'yellow',
'5',
'Tom',
'100',]
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For many of the other submit/evaluate calls you've left the transaction name on the first line and put arguments on subsequent lines. Whatever style you prefer is fine, it just might be nice to consistently use the same style throughout.

Comment on lines 192 to 195
} catch (error: unknown) {
if (!(error instanceof GatewayError) || error.code !== grpc.status.CANCELLED) {
throw error;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't expect the event stream to be closed until after we finish reading and break out of the loop (above), so should probably not be catching any errors here. Just remove the catch.

await deleteAssetByID(contract);

// Replay all the events received.
await Promise.race([timeout(10000),replayChaincodeEvents(network,firstBlockNumber)])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate you're replicating the read timeout in the Go sample. That was more to demonstrate different patterns of Go channel usage for event processing, and was a per-event timeout rather than for the whole sequence of reads.

If you want to keep this timeout example (which is reasonable), you probably need to change the logical flow slightly so you have the CloseableAsyncIterable<ChaincodeEvent> (or at least the close() function from it) from this event reading session and can close it if the timeout fires.

Implementing completely cleanly is going to be a little more complex so you might want to take one of these alternative approaches:

  1. Set a deadline CallOption for the call to get chaincode events, so gRPC will timeout the connection at the deadline for you.
  2. Don't worry about the timeout.

It's good practice to have some kind of timeout so the client application won't ever hang indefinitely but, if it's looking complicated to achieve that, it might be better to keep things simple so the sample code is as readable as possible.

Readme update
Wait for events to complete
Refactor code for events replay
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
@mbwhite mbwhite merged commit 1a79d13 into hyperledger:main Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants