feat(backend): add middleware that handles STREAM KYC/additional data frames#3706
Conversation
❌ Deploy Preview for brilliant-pasca-3e80ec failed. Why did it fail? →
|
mkurapov
left a comment
There was a problem hiding this comment.
Only focused on the kyc decision middleware here
|
|
||
| const readDecision = async (): Promise<string | undefined> => { | ||
| try { | ||
| const value = await redis.get(cacheKey) |
There was a problem hiding this comment.
We have a poll utility function for this in utils.ts
|
|
||
| const incomingPaymentId = ctx.state.streamDestination | ||
| const connectionId = ctx.state.connectionId ?? 'unknown' | ||
| const cacheKey = `kyc_decision:${incomingPaymentId}:${connectionId}` |
There was a problem hiding this comment.
I think we should incorporate the polling inside the existing incoming payment service that will handle the logic for this. That is where we can also put the code to actually confirm or reject the partial incoming payment.
There was a problem hiding this comment.
I think the incoming payment service is fine for this one.
| } | ||
|
|
||
| const incomingPaymentId = ctx.state.streamDestination | ||
| const connectionId = ctx.state.connectionId ?? 'unknown' |
There was a problem hiding this comment.
Let's generate our own UUID partialIncomingPaymentId
| import { ILPContext, ILPMiddleware } from '../rafiki' | ||
| import { StreamState } from './stream-address' | ||
|
|
||
| export function createKycDecisionMiddleware(): ILPMiddleware { |
There was a problem hiding this comment.
I think we should not have any references to "kyc" in this code change, but simply partialPaymentDecisionMiddleware or something
| createIncomingErrorHandlerMiddleware(ilpAddress), | ||
| createStreamAddressMiddleware(), | ||
|
|
||
| createKycDecisionMiddleware(), |
There was a problem hiding this comment.
should we move this to be after the createBalanceMiddleware ? That way, we will only publish the webhook & start polling for the decision after we were able to created a pending transfer between the peer account to the incoming payment account, confirming we have enough funds to make the transfer anyway.
| dataToTransmit?: string | ||
| ): Promise<IncomingPayment | IncomingPaymentError> { | ||
| const { config, knex } = deps | ||
| options?: { |
There was a problem hiding this comment.
| options?: { | |
| options: { |
(since we would only call this function when it is defined)
| request: async (): Promise<string> => { | ||
| try { | ||
| const value = await redis.get(cacheKey) | ||
| return value ?? '' |
There was a problem hiding this comment.
I think we should expect the value to be stringified { approved: boolean, reason?: string }), and we can then just check for approved boolean (see #3687)
| createIncomingErrorHandlerMiddleware(ilpAddress), | ||
| createStreamAddressMiddleware(), | ||
|
|
||
| createPartialPaymentDecisionMiddleware(), |
There was a problem hiding this comment.
should we move this to be after the createBalanceMiddleware ? That way, we will only publish the webhook & start polling for the decision after we were able to created a pending transfer between the peer account to the incoming payment account, confirming we have enough funds to make the transfer anyway.
| plugin, | ||
| destination, | ||
| quote, | ||
| appData: Buffer.from('hello kyc') |
There was a problem hiding this comment.
We can use the outgoing payments' dataToTransmit here now I think
| plugin, | ||
| destination, | ||
| quote, | ||
| appData: Buffer.from('hello kyc') |
There was a problem hiding this comment.
We can use the outgoing payments' dataToTransmit here now I think
…artial payments to mock ASE
🚀 Performance Test ResultsTest Configuration:
Test Metrics:
📜 Logs |
| "@interledger/stream-receiver": "0.3.3-alpha.4", | ||
| "@interledger/pay": "0.4.0-alpha.10", |
There was a problem hiding this comment.
Let's put these just in the backend package (unless I'm mistaken and they are used elsewhere too)
| destination, | ||
| quote, | ||
| appData: outgoingPayment.dataToTransmit | ||
| ? Buffer.from(outgoingPayment.dataToTransmit, 'utf8') |
There was a problem hiding this comment.
if you use outgoingPayment.getDataToTransmit() it will be decrypted, such that the reciever won't have to do that. It is already encrypted via STREAM
| createIncomingErrorHandlerMiddleware(ilpAddress), | ||
| createStreamAddressMiddleware(), | ||
|
|
||
| createPartialPaymentDecisionMiddleware(), |
| message ?? 'Error processing partial payment', | ||
| 'utf8' | ||
| ) | ||
| ctx.response.reply = replyOrMoney.finalDecline(errorData) |
There was a problem hiding this comment.
What are the consequences here if we do not throw (like before)?
There was a problem hiding this comment.
When setting the reply by calling finalDecline we respond to the sender with a final F99 error (along with the error message coming from the ASE/Rafiki) and closing frame on the reject. Throwing was not correct IMO since purposefully rejecting a payment shouldn't be treated as an unexpected error i.e. handled by the error middleware.
| ) | ||
| return false | ||
| } | ||
| const cacheKey = `${PARTIAL_PAYMENT_DECISION_PREFIX}:${options.id}:${options.partialPaymentId}` |
There was a problem hiding this comment.
Maybe let's have a separate small function for this so it's consistent in both methods
| 'SEND_TENANT_WEBHOOKS_TO_OPERATOR', | ||
| false | ||
| ), | ||
| partialPaymentDecisionMaxWaitMs: envInt( |
There was a problem hiding this comment.
Maybe we should have the decision behaviour (send webhook & poll) false by default (even if there are additional data frames on the packet). What do you think? The ASE could enable it smoothly.
| | `ENABLE_AUTO_PEERING` | `backend.enable.autoPeering` | `false` | When `true`, auto-peering is enabled. | | ||
| | `ENABLE_MANUAL_MIGRATIONS` | `backend.enableManualMigrations` | `false` | When `true`, you must run the database manually with the command `npm run knex – migrate:latest –env production` | | ||
| | `ENABLE_SPSP_PAYMENT_POINTERS` | `backend.enable.spspPaymentPointers` | `true` | When `true`, the SPSP route is enabled. | | ||
| | `DB_ENCRYPTION_SECRET` | _undefined_ | _undefined_ | Base64-encoded 32-byte secret used to encrypt/decrypt transmitted payment data (`dataToTransmit`) stored on incoming/outgoing payment records and events. | |
There was a problem hiding this comment.
We don't need to include it in the v1-beta docs
| const { cipherText, tag, iv } = JSON.parse(rawDataToTransmit) as { | ||
| cipherText: string | ||
| tag: string | ||
| iv: string | ||
| } | ||
|
|
||
| const decipher = createDecipheriv( | ||
| 'aes-256-gcm', | ||
| Uint8Array.from(Buffer.from(dbEncryptionSecret, 'base64')), | ||
| iv | ||
| ) | ||
| decipher.setAuthTag(Uint8Array.from(Buffer.from(tag, 'base64'))) | ||
| decipher.update(cipherText, 'base64', 'utf8') | ||
| decipher.final('utf8') |
There was a problem hiding this comment.
Do you think we should decrypt this data before publishing the webhook?
| walletAddressService: WalletAddressService | ||
| assetService: AssetService | ||
| config: IAppConfig | ||
| redis?: Redis |
There was a problem hiding this comment.
What are the consequences of making this required?
There was a problem hiding this comment.
None, I made it required 👍
There was a problem hiding this comment.
Do we need to change the specs here? or was it leftover from a merge?
There was a problem hiding this comment.
Leftover from the rebase, fixed it in the latest commit
| partialIncomingPaymentId: uuid(), | ||
| expiresAt: prepare.expiresAt | ||
| } | ||
| )) as PartialPaymentDecision |
There was a problem hiding this comment.
| )) as PartialPaymentDecision | |
| )) |
| options?: { | ||
| dataToTransmit?: string | ||
| partialIncomingPaymentId?: string | ||
| expiresAt?: Date | ||
| } | ||
| ): Promise<PartialPaymentDecision> |
There was a problem hiding this comment.
| options?: { | |
| dataToTransmit?: string | |
| partialIncomingPaymentId?: string | |
| expiresAt?: Date | |
| } | |
| ): Promise<PartialPaymentDecision> | |
| options: { | |
| dataToTransmit: string | |
| partialIncomingPaymentId: string | |
| expiresAt: Date | |
| } | |
| ): Promise<PartialPaymentDecision> |
e8c8822
into
feature/encrypted-data-exchange
… frames (#3706) * feat(backend): add middleware that handles STREAM KYC/additional data payloads - wip * feat(backend): add wip comment * feat: support arbitrary data on prepare packets - WIP * feat(pay): add controller to handle additional data in STREAM packets * feat: add STREAM connection id in cache key for KYC response, remove unused env vars * chore: address PR comments * chore: add tests for payment decision middleware and interledgerjs changes * fix: update app data controller ti return F99 * feat: update additional data middleware logic, add pay and stream-receiver updates * chore(backend): use updated pay and stream-receiver from npm, update lockfile * feat(backend): refactor partial payment middleware, add handler for partial payments to mock ASE * fix(backend): fix build errors in CI * fix(backend): formatting * fix(backend): formatting * fix(backend): partial payment decision middleware tests * fix(backend): formatting * feat(backend): address review comments * feat(backend): address PR comments * fix(backend): fix failing CI tests * chore: align open-payments-specifications with feature/encrypted-data-exchange * fix: webhooks tests * fix: delete flaky test * fix: backend build * chore: address review comments
* feat(backend): update sender data in incoming partial payment webhook (#3810) * feat: update sender data in incoming partial payment webhook * feat(backend): update partial payment payload in webhook * feat: uuid to id, use args object * feat(backend): add confirmPartialIncomingPayment resolver (#3819) * feat: update sender data in incoming partial payment webhook * feat(backend): update partial payment payload in webhook * feat: uuid to id, use args object * feat(backend): add confirmPartialIncomingPayment resolver * feat: move return, revert jest env * chore: rename kyc decision prefix * feat(backend): reject partial payment gql api (#3823) * feat(backend): add confirmPartialIncomingPayment resolver * feat: move return, revert jest env * feat(backend): reject partial payment gql api * fix: use correct redis key * fix: use const * feat: move redis call out of resolver * feat(backend): add middleware that handles STREAM KYC/additional data frames (#3706) * feat(backend): add middleware that handles STREAM KYC/additional data payloads - wip * feat(backend): add wip comment * feat: support arbitrary data on prepare packets - WIP * feat(pay): add controller to handle additional data in STREAM packets * feat: add STREAM connection id in cache key for KYC response, remove unused env vars * chore: address PR comments * chore: add tests for payment decision middleware and interledgerjs changes * fix: update app data controller ti return F99 * feat: update additional data middleware logic, add pay and stream-receiver updates * chore(backend): use updated pay and stream-receiver from npm, update lockfile * feat(backend): refactor partial payment middleware, add handler for partial payments to mock ASE * fix(backend): fix build errors in CI * fix(backend): formatting * fix(backend): formatting * fix(backend): partial payment decision middleware tests * fix(backend): formatting * feat(backend): address review comments * feat(backend): address PR comments * fix(backend): fix failing CI tests * chore: align open-payments-specifications with feature/encrypted-data-exchange * fix: webhooks tests * fix: delete flaky test * fix: backend build * chore: address review comments * fix: cherrypick 4917c14 * chore: format * fix: remove duplicate property * fix: backend tests * chore: update lock file * chore: format * chore: update lock file * fix: attempt to fix CI builds * fix: address PR comments * feat: add encrypted data exchange docker compose variant and manual test doc (#3905) * feat: add partial payment rejection description on outgoing_payment.failed webhook * fix: pin testcontainers to 10.16.0 for Tigerbeetle tests * fix: attempt to fix CI backend build * fix: revert testcontainers version changes and update lockfile * chore: format * fix: missing error message in outgoing payment tests * chore: minor updates for encrypted data exchange feature (#3911) * doc: update encrypted data exchange doc * chore: rename dataToTransmit to dataFromSender on recipient side * chore: rename updatePartialPaymentDecision args * test: add test for updatePartialPaymentDecision and for processPartialPayment * chore: make the partial payment decision success case more explicit * test: rename dataToTransmit to dataFromSender * test(backend): updates partial payment decision middleware tests * chore: update pay library version * chore: update pay library to alpha 12 * fix: use dataFromSender instead dataToTransmit when decrypting webhook data * chore: update localenv doc * chore: update RejectPartialIncomingPaymentInput reason description Co-authored-by: Max Kurapov <max@interledger.org> * chore: regenerate graphql types --------- Co-authored-by: Nathan Lie <lie4nathan@gmail.com> Co-authored-by: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Co-authored-by: Max Kurapov <max@interledger.org>
Changes proposed in this pull request
Context
Closes #RAF-1185
Checklist
fixes #numberuser-docslabel (if necessary)