Send message when transfer finished#244
Send message when transfer finished#244dadiorchen wants to merge 16 commits intoGreenstand:masterfrom
Conversation
| @@ -43,4 +45,49 @@ describe('TransferService', () => { | |||
| expect(result).property('originating_wallet').eq('testName'); | |||
| expect(result).property('destination_wallet').eq('testName'); | |||
There was a problem hiding this comment.
While not very significant, it will be nice to have different name for destination_wallet to differentiate the origination_wallet and destination_wallet.
Isn't something like this possible to make sinon stub return specific Wallet objects for originator wallet and desintation wallet so that we can differentiate them.
sinon.stub(WalletService.prototype, 'getById').withArgs(walletID1).resolves(
new Wallet({ id: walletId1, name: 'originWallet' }) )
sinon.stub(WalletService.prototype, 'getById').withArgs(walletID1).resolves(
new Wallet({ id: walletId2, name: 'desinationWallet' }) )
There was a problem hiding this comment.
Yes, you are right, useless checking for that, should be changed.
Thanks, good suggestion, withArgs
| }); | ||
| } | ||
|
|
||
| async getTokensById(id){ |
There was a problem hiding this comment.
Renaming this to getTokenAndCaptureIds(transactionId) would be verbose but more clear. Right now, it is not clear the parameter id is referring the transaction/transfer id or token id.
Another lesser preference would be to rename it to getTokens(transactionId)
There was a problem hiding this comment.
Good, I agree, I will change it
arunbakt
left a comment
arunbakt
left a comment
There was a problem hiding this comment.
Hi @dadiorchen , Left few comments regarding
- the message queue config
- dealing with sending a message transitionally by doing it in two separate process
I haven't completed the review, I still have to go through MQService.js and TransferService.js.
But some of the current feedback might be useful to look into. We can chat if you aren't clear on any of the things I am talking about.
Thank you!
Arun
| tracker.on('query', function sendResult(query, step) { | ||
| [ | ||
| function firstQuery() { | ||
| expect(query.sql).match(/capture_id/); |
There was a problem hiding this comment.
Is match here a regex kind? I feel either we should do a robust match by verifying the exact query if possible.
Even if we did this test seems to be verifying just the method signature and knex but nothing significant in our code. I would prefer to skip the unit test for this Repository class if you ask me. It would be beneficial and reduce our developer time spent on maintaining it.
There was a problem hiding this comment.
Hmm, maybe you are right, I don't realize that I added some tests maybe look not so useful, yes, it might be unnessarary, I will think it over, and another reason lead to this result is that I wrote the test first before I wrote the implementation, so when I link this model to the real system, I don't need to worry too much about that it's some problematic model, it makes integrating the code easier.
| if (result.state === Transfer.STATE.completed) { | ||
| // just send message in production | ||
| if(process.env.NODE_ENV !== "test"){ | ||
| await transferService.sendMessage(result.id); |
There was a problem hiding this comment.
One way we could make this a bit more robust is to raise the messages as a domain-event (a separate table that stores all the events raised) with a payload that holds the message and a status(that can be raised, sent).
The raised domain-event will be stored and should be in the same transaction along with the transfer logic.
After the first part is complete, we could proceed to dispatch the domain event payload (the message) to the queue and mark the domain-event as sent in a second db call that probably doesn't need to be in a transaction since it deals with just one table.
There was a problem hiding this comment.
Do we have some 3rd party libraries to implement this pattern? Or we should build a module about this by ourselves? I think this might be a common demand in different our project?
There was a problem hiding this comment.
You are right, we need this in whichever service we emit/receive messages and store/process them. I started a separate project (to be treated as a submodule) https://github.com/Greenstand/domain-events. I am not sure if there is a NPM library that can meet our needs exactly. It might easier to use our own given we also need to store these events in a database.
server/services/MQConfig.js
Outdated
| "timeout": 1000 | ||
| } | ||
| }, | ||
| "exchanges": ["field-data"], |
There was a problem hiding this comment.
You probably need a new exchange named wallet-service-ex with a queue binding like the following. The consumer of your publication is web-map-query-service. "field-data-events" queue are meant for events generated by field-data-service.
"exchanges": [
"wallet-service-ex"
],
"queues": [
"token-transfer:events"
],
"bindings": [
"wallet-service-ex[token.transfer] -> token-transfer: events"
],
"publications": {
"token-assigned": {
"exchange": "wallet-service-ex",
"routingKey": "token.transfer"
}
}
There was a problem hiding this comment.
BTW, do we have something like a dashboard/panel to check and manage the queue? RabbitMQ I mean? How can I know what kind of exchanges are existing?
There was a problem hiding this comment.
Yes we do. I will ping you the url and credentials separately. Also, if the rabbitmq doesn't have an exchange and queues that you have specified in the config, it gets created when app starts running.
There was a problem hiding this comment.
Thanks, @arunbakt for your helpful configuration. I have added the exchange and queue, now the message has been sent to the queue,
2 participants
No description provided.