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

feat(mojaloop/#3498): alter message keys for prepare, fulfil and timeout #965

Merged
merged 18 commits into from
Sep 4, 2023

Conversation

kleyow
Copy link
Contributor

@kleyow kleyow commented Aug 30, 2023

feat(mojaloop/#3498): alter message keys for prepare, fulfil and timeout - mojaloop/project#3498

  • Refator central-services-shared proceed to lib with simplified keying logic
  • Remove keying from some NOTIFICATION messages in transfers handler
  • Keyed successful position prepares by payer account id
  • Keyed successful position fulfils by payee account id
  • Keyed all position aborts by payer account id

@kleyow kleyow marked this pull request as ready for review August 31, 2023 14:13
src/handlers/timeouts/handler.js Outdated Show resolved Hide resolved
src/handlers/timeouts/handler.js Outdated Show resolved Hide resolved
@@ -198,7 +198,9 @@ const prepare = async (error, messages) => {
Logger.isInfoEnabled && Logger.info(Util.breadcrumb(location, `positionTopic1--${actionLetter}7`))
functionality = TransferEventType.POSITION
const eventDetail = { functionality, action }
await Kafka.proceed(Config.KAFKA_CONFIG, params, { consumerCommit, eventDetail, toDestination })
// Key position prepare message with payer account id
const payerAccount = await Participant.getAccountByNameAndCurrency(payload.payerFsp, payload.amount.currency, Enum.Accounts.LedgerAccountType.POSITION)
Copy link
Contributor

Choose a reason for hiding this comment

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

For this particular case here(happy path), if we want to save a DB query, the participant account id is already fetched in saveTransferPrepared of facade. You can pass it down to access here. But as long as the query is cached, I don't think its a must. @mdebarros What do you suggest?

Copy link
Member

@mdebarros mdebarros Sep 4, 2023

Choose a reason for hiding this comment

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

Lets create a story for this. The reason I say that is because this in practice shouldn't be an issue as long as caching is enabled.

We should definitely look at such improvements going forward. A dedicated story may also give us the opportunities to reduce other similar duplicate queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created a story to optimize duplicated queries for participant currency id with a clause to investigate other instances of duplicated queries (still updating the story, just a placeholder) mojaloop/project#3515

Copy link
Member

@mdebarros mdebarros left a comment

Choose a reason for hiding this comment

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

+1

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.

3 participants