Use separate eventstream per namespace #135
Conversation
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
nguyer
requested review from
awrichar,
peterbroadhurst and
shorsher
as code owners
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
| @@ -15,7 +15,7 @@ | |||
| // limitations under the License. | |||
|
|
|||
| import { BadRequestException, Injectable, Logger } from '@nestjs/common'; | |||
| import * as LRUCache from 'lru-cache'; | |||
| import LRUCache from 'lru-cache'; | |||
There was a problem hiding this comment.
Definitely make sure the Docker still works with this. We've seen weird differences in Node depending on the default import syntax.
| @@ -1,5 +1,5 @@ | |||
| PORT=3000 | |||
| ETHCONNECT_URL=http://127.0.0.1:5102 | |||
| ETHCONNECT_INSTANCE=/contracts/erc1155 | |||
There was a problem hiding this comment.
Wow, this one is old. Why did it pop back up? Do we still rely on it anywhere? Definitely tried to move to CONTRACT_ADDRESS a while back.
src/tokens/tokens.service.ts
Outdated
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // http://www.apache.org/licenses/LICENSE -2.0 |
src/tokens/tokens.service.ts
Outdated
| await this.createPoolSubscription(ctx, defaultContract, BASE_SUBSCRIPTION_NAME); | ||
| } | ||
| } | ||
| async onConnect() {} |
There was a problem hiding this comment.
I think this can be removed, along with the supporting methods that wanted it to exist...
| this.logger.log('Using event stream with name ' + name); | ||
| this.stream = await this.eventstream.createOrUpdateStream(ctx, name, name); | ||
| return this.stream; | ||
| await this.migrationCheck(ctx); |
There was a problem hiding this comment.
As in ERC20, I think this should be removed.
There was a problem hiding this comment.
Actually... looks like you have already repurposed it in this case to do the purge instead of just the check. That's probably fine.
| this.currentClient = undefined; | ||
| // Iterate over all the namespaces this client was subscribed to | ||
| this.namespaceClients.forEach((clientSet, namespace) => { | ||
| clientSet.delete(client); |
There was a problem hiding this comment.
Same note as ERC20 - shouldn't we check the return to see if the clientSet actually included this client?
There was a problem hiding this comment.
I think now that awaitingAck is scoped to the client itself, none of the rest of this loop will have any effect if the client wasn't listening on the namespace
| @@ -186,6 +195,7 @@ export class TokenListener implements EventListener { | |||
| interfaceFormat: InterfaceFormat.ABI, | |||
| poolData: unpackedSub.poolData, | |||
| poolLocator: packedPoolLocator, | |||
| alternateLocators: oldLocator ? [oldLocator] : [], | |||
There was a problem hiding this comment.
Glad this turned out to be a (relatively) simple migration path.
| @@ -50,6 +50,18 @@ export interface WebSocketMessage { | |||
| data: any; | |||
There was a problem hiding this comment.
Can delete this type if it's unused.
There was a problem hiding this comment.
Looks good - basically all the same feedback as hyperledger/firefly-tokens-erc20-erc721#140. Thanks for figuring out the weirdness with the pool locator migration on this one too.
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Parallel to hyperledger/firefly-tokens-erc20-erc721#140