Implement patch notification for failure scenarios (following v1.1 update)

[elnyry-sam-k]

Goal:

As a Payee FSP

I want to be able to receive a notification upon finalization of a transfer even in failure cases (success supported now)

so that I can use the notification to mitigate risk on payments to end-users

Acceptance Criteria:

• A PATCH notification is sent to the Payee FSP when a transfer is ABORTED
• Tests are added in the TTK GP test cases to cover the various scenarios using this patch notification (using transfer states reserved, committed when transfer is aborted, committed, etc)

Complexity: High

Uncertainty: Low

---

Tasks:

• Review spec for pattern and existing implementation of patch notification on success cases
• Implementation of patch on failure cases (when transfer is ABORTED)
  • central-ledger changes
  • central-services-shared changes
  • ml-api-adapter changes
• Add TTK tests for failure notification with v1.1 (Payee fsp receives notification when a transfer is ABORTED; not just when a transfer is COMMITTED):
  • When a PUT /transfers/{ID} is sent from Payee FSP -> Switch with a status of ABORTED (invalid value), there is no PATCH /transfers/{ID} sent to the Payee FSP (this is an error condition covered in 1.2)
  • When a PUT /transfers/{ID} is sent from Payee FSP -> Switch with a status of RESERVED, and the transfer completes successfully, a PATCH /transfers/{ID} is sent to the Payee FSP with a status of COMMITTED (this is the existing case)
  • When a PUT /transfers/{ID} is sent from Payee FSP -> Switch with a status of RESERVED, and the transfer has a fulfilment that doesn't match the condition, the switch aborts the transfer and a PATCH /transfers/{ID} is sent to the Payee FSP with a status of ABORTED (new functionality)
  • where an invalid value is used for the completedTimestamp of the PUT /transfers/{ID} request by the Payee FSP; we have some validations on the switch which check for this so this can fail as well. (new functionality)
  • When a Payee FSP does not send a PUT /transfers/{ID}, and the transfer times out, no PATCH /transfers/{ID} is sent to the Payee FSP

Done

• Acceptance Criteria pass
• Designs are up-to date
• Unit Tests pass
• Integration Tests pass
• Code Style & Coverage meets standards
• Changes made to config (default.json) are broadcast to team and follow-up tasks added to update helm charts and other deployment config.

Pull Requests:

• fix(vulns): run npm audit, fix mpath object-store-lib#33
• chore(refactor): tidy up fulfil handler code central-ledger#870
• feat: add a new action enum for RESERVED_ABORTED central-services-shared#317
• feat(mojaloop/project#2553): patch notification for Payee central-services-shared#318 --> feat(mojaloop/project#2556): implement patch notification for failure scenarios (following v1.1 update) central-services-shared#321
• fix(mojaloop/project/issue#2677): npm install on central-ledger fails with central-services-shared v15.3.0 dependency central-services-shared#322
• feat: add RESERVED_ABORTED to notification handler ml-api-adapter#489 --> feat(mojaloop/project#2556): implement patch notification for failure scenarios ml-api-adapter#492
• feat(mojaloop/project#2553): send extra notification back to payee when a transfer is aborted central-ledger#872 --> feat(mojaloop/project#2556): implement patch notification for failure scenarios central-ledger#874
• feat: callback rule updates to suppose patch notification test-scenarios ml-testing-toolkit#200
• feat: patch notification test-scenarios testing-toolkit-test-cases#61

Follow-up:

• N/A

Dependencies:

• N/A

Accountability:

• Owner: TBC
• QA/Review: TBC

[lewisdaly]

@mdebarros and I talked about this yesterday.

There isn't enough information in the different ABORT_* notifications for the ml-api-adapter to be able to infer whether or not it should send a PATCH /transfers/{ID} to the Payee with a status of ABORTED.

For this reason, we decided that the central-ledger should emit a new notification message for this case, and the ml-api-adapter, if it's API version is 1.1 should then send this message as a PATCH /transfers/{ID} to the Payee with a status of ABORTED.

[lewisdaly]

@elnyry-sam-k @mdebarros I've written out 4 test cases above for the feature - can you please check that these are correct and add any that you think I've missed?

[elnyry-sam-k]

hi Lewis, thanks for noting these - it helps the discussion..

1. When a PUT /transfers/{ID} is sent from Payee FSP -> Switch with a status of ABORTED, there is no extra PATCH /transfers/{ID} sent to the Payee FSP
2. When a PUT /transfers/{ID} is sent from Payee FSP -> Switch with a status of RESERVED, and the transfer completes successfully, a PATCH /transfers/{ID} is sent to the Payee FSP with a status of COMMITTED (this is the existing case)
3. When a PUT /transfers/{ID} is sent from Payee FSP -> Switch with a status of RESERVED, and the transfer has a fulfilment that doesn't match the condition, the switch aborts the transfer and a PATCH /transfers/{ID} is sent to the Payee FSP with a status of ABORTED
4. When a Payee FSP does not send a PUT /transfers/{ID}, and the transfer times out, no PATCH /transfers/{ID} is sent to the Payee FSP

Cases 1 & 3 fall under the same category - invalid values being sent for required fields; from FSPIOP v1.1 onwards, 'ABORTED' is an invalid value if used as a transferState in a fulfil request (the Payee FSP should use an error callback, a PUT on /transfers/{ID}/error for this) and both these cases should now have this new behavior that we are introducing (in implementation) - the sending of PATCH with transferState as ABORTED.

As you pointed out option-2 should be currently existing functionality.

The last one option-4 is an interesting one. Right now yes, we're not sending a PATCH but we do PUT /transfers/{ID}/error error callbacks to both the Payer & Payee FSPs. This is not inline with the spec; but maybe its best to leave it as it is for now; lets discuss on one of the meetings and add a backlog item for item-4 while continuing for now as-is on it.

[lewisdaly]

I'm not sure I follow your logic for #1. If it's invalid in the FSPIOP v1.1 API, option #1 should return a synchronous response such as 400 Bad Request.

[elnyry-sam-k]

I'm not sure I follow your logic for #1. If it's invalid in the FSPIOP v1.1 API, option #1 should return a synchronous response such as 400 Bad Request.

There cannot be a synchronous response here (following schema validation) because ABORTED is a valid state for a transfer in general and it can be used in a PUT callback when it is sent in response to a GET /transfers/{ID}, but Payee FSPs cannot use this value to abort a transfer, they should use the error callback. More on this here: mojaloop/mojaloop-specification#47 (comment)

[elnyry-sam-k]

@lewisdaly - In addition to the four scenarios you added, I'd suggest a scenario-5 where an invalid value is used for the completedTimestamp of the PUT /transfers/{ID} request by the Payee FSP; we have some validations on the switch which check for this so this can fail as well.

[lewisdaly]

There cannot be a synchronous response here (following schema validation) because ABORTED is a valid state for a transfer in general and it can be used in a PUT callback when it is sent in response to a GET /transfers/{ID}

That's annoying. I still don't understand why that case should result in a PATCH /transfers/{ID} from the switch to the Payee though. I thought we only send the final status of the transfer in a PATCH /transfers/{ID} when the Payer FSP sends a PUT /transfers/{ID} with a status of RESERVED

[elnyry-sam-k]

the last part is still correct Lewis, yes. As part of this story - we're looking at the patch only for transfers that use RESERVED state in the fulfil request

[lewisdaly]

I'm having trouble locating the existing e2e test that demonstrates the payee FSP sending a RESERVED status, and expecting a PATCH /transfers/{ID} callback from the switch. @elnyry-sam-k or @vijayg10 can you please help me find it? That's the one I plan on extending for this.

[lewisdaly]

I'm having trouble writing E2E tests for this - see #2624

[elnyry-sam-k]

hi @lewisdaly , will discuss this with Vijay today

[lewisdaly]

I'm not able to continue working on this ticket.

I got up to the point where I was writing the above TTK tests, and ran into issues setting up custom rules to help simulate the different scenarios. We later determined that this was an issue only on firefox, so this is no longer blocked.

Once we have the updated TTK E2E tests, then we can be confident that this is working as expected, and finish up those open PRs.

[mdebarros]

@lewisdaly, would you be so kind to give me write permissions to your forked repos so I can continue the work on this?

I.e. resolving merge conflicts, etc.

I would like to finalise these PRs ASAP!

[lewisdaly]

Hey it looks like you've just recreated my branches elsewhere? So you don't need me to do this?

[mdebarros]

Hey it looks like you've just recreated my branches elsewhere? So you don't need me to do this?

Yeah....decided to go with that approach. Sorry about that. Needed to get these PRs sorted, and it sounded like you were not going to be available for awhile due to travel :D/

[lewisdaly]

Nah all good - thanks for sorting it out :)