Skip to content

feat(backend): try to match 'wallet address not found' webhook to tenant by prefix #3426

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 27, 2025
Merged

feat(backend): try to match 'wallet address not found' webhook to tenant by prefix #3426

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a4dfc3
feat(backend): try to match 'wallet address not found' webhook to ten…
njlie May 8, 2025
c103459
fix: negative test for prefix matching
njlie May 19, 2025
aa2d39f
fix: tests and withConfigOverride usage
njlie May 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion packages/backend/src/open_payments/wallet_address/service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ describe('Open Payments Wallet Address Service', (): void => {
await appContainer.shutdown()
})

describe('Create or Get Wallet Address3', (): void => {
describe('Create or Get Wallet Address', (): void => {
let tenantId: string
let options: CreateOptions

Expand Down Expand Up @@ -606,6 +606,53 @@ describe('Open Payments Wallet Address Service', (): void => {
)
)

test('creates wallet address not found event for tenant with matching prefix', async (): Promise<void> => {
withConfigOverride(
() => config,
{ walletAddressLookupTimeoutMs: 0 },
async (): Promise<void> => {
const walletAddressUrl = `https://${faker.internet.domainName()}/.well-known/pay`
await expect(
walletAddressService.getOrPollByUrl(walletAddressUrl)
).resolves.toBeUndefined()

const tenant = await createTenant(deps)
await createTenantSettings(deps, {
tenantId: tenant.id,
setting: [
{
key: TenantSettingKeys.WALLET_ADDRESS_URL.name,
value: new URL(walletAddressUrl).origin
}
]
})

const walletAddressNotFoundEvents = await WalletAddressEvent.query(
knex
)
.where({
type: WalletAddressEventType.WalletAddressNotFound
})
.withGraphFetched('webhooks')

expect(walletAddressNotFoundEvents).toMatchObject({
data: { walletAddressUrl },
webhooks: [
expect.objectContaining({
recipientTenantId: config.operatorTenantId,
eventId: walletAddressNotFoundEvents[0].id,
processAt: expect.any(Date)
}),
expect.objectContaining({
recipientTenantId: tenant.id,
eventId: walletAddressNotFoundEvents[0].id,
processAt: expect.any(Date)
})
]
})
}
)
})
Copy link
Contributor

@BlairCurrey BlairCurrey May 20, 2025

Choose a reason for hiding this comment

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

Oh boy. We've been doing something wrong with withConfigOverride.

This never fails. Throw an error, do expect(true).toBe(false), etc. and you will see it pass.

We need to invoke withConfigOverride (and also await it). Like await withConfigOverride(...)().

Outside the scope of this task but we should fix the other usages of withConfigOverride. Alternatively, instead of evoking it, we could change withConfigOverride to work like we've been expecting it to. Just calling the test directly instead of returning a function.

export async function withConfigOverride(
  getConfig: () => IAppConfig,
  configOverride: Partial<IAppConfig>,
  test: jest.Func
): Promise<void> {
  const config = getConfig()
  const savedConfig = Object.assign({}, config)

  Object.assign(config, configOverride)

  try {
    await test()
  } finally {
    Object.assign(config, savedConfig)
  }
}

I did not test this and would not be surprised if this causes some tests to fail (which were previously incorrectly passing). Also note that our current implementation passes in some arg: any[] to the test fn but I dont think its actually used anywhere nor do I think we'd even need to use it.

Copy link
Contributor

@BlairCurrey BlairCurrey May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh shoot, that's not 100% right. Was comparing to another example that was working fine.

we just need to change it to this:

      test('creates wallet address not found event for tenant with matching prefix', 
        withConfigOverride(

from this

      test('creates wallet address not found event for tenant with matching prefix', async (): Promise<void> => {
        withConfigOverride(

The second argument to the jest test should just be withConfigOverride. not an async function with withConfigOverride in it. So I think it's just this one an not necessarily a widespread problem with our use of withConfigOverride.

Copy link
Contributor Author

@njlie njlie May 21, 2025

Choose a reason for hiding this comment

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

Thanks for catching this! Glad it's just contained to this PR.

test(
'polls for wallet address',
withConfigOverride(
Expand Down
14 changes: 13 additions & 1 deletion packages/backend/src/open_payments/wallet_address/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,13 +379,25 @@ async function getOrPollByUrl(
const existingWalletAddress = await getWalletAddressByUrl(deps, url)
if (existingWalletAddress) return existingWalletAddress

let containsOperatorTenant = false
const webhookRecipients = (
await deps.tenantSettingService.getSettingsByPrefix(url)
).map((tenantSetting) => {
if (tenantSetting.tenantId === deps.config.operatorTenantId)
containsOperatorTenant = true
return { recipientTenantId: tenantSetting.tenantId }
})

if (!containsOperatorTenant)
webhookRecipients.push({ recipientTenantId: deps.config.operatorTenantId })

await WalletAddressEvent.query(deps.knex).insertGraph({
type: WalletAddressEventType.WalletAddressNotFound,
data: {
walletAddressUrl: url
},
tenantId: deps.config.operatorTenantId,
webhooks: [{ recipientTenantId: deps.config.operatorTenantId }]
webhooks: webhookRecipients
})

deps.logger.debug(
Expand Down
37 changes: 36 additions & 1 deletion packages/backend/src/tenants/settings/service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ import { Tenant } from '../model'
import { TenantService } from '../service'
import { faker } from '@faker-js/faker'
import { exchangeRatesSetting, randomSetting } from '../../tests/tenantSettings'
import { TenantSetting } from './model'
import { TenantSetting, TenantSettingKeys } from './model'
import {
CreateOptions,
GetOptions,
TenantSettingService,
UpdateOptions
} from './service'
import { AuthServiceClient } from '../../auth-service-client/client'
import { v4 as uuid } from 'uuid'
import { createTenant } from '../../tests/tenant'

describe('TenantSetting Service', (): void => {
let deps: IocContract<AppServices>
Expand Down Expand Up @@ -371,4 +373,37 @@ describe('TenantSetting Service', (): void => {
expect(result).toEqual([])
})
})

describe('get settings by value', (): void => {
test('can get settings by wallet address prefix setting', async (): Promise<void> => {
Copy link
Contributor

@mkurapov mkurapov May 17, 2025

Choose a reason for hiding this comment

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

we should add a negative test to check that we avoid fetching a tenant if the prefix doesn't match

const secondTenant = await createTenant(deps)
const baseUrl = `https://${faker.internet.domainName()}/${uuid()}`
const settings = (
await Promise.all([
tenantSettingService.create({
tenantId: tenant.id,
setting: [
{
key: TenantSettingKeys.WALLET_ADDRESS_URL.name,
value: `${baseUrl}/${uuid()}`
}
]
}),
tenantSettingService.create({
tenantId: secondTenant.id,
setting: [
{
key: TenantSettingKeys.WALLET_ADDRESS_URL.name,
value: `${baseUrl}/${uuid()}`
}
]
})
])
).flat()

const retrievedSettings =
await tenantSettingService.getSettingsByPrefix(baseUrl)
expect(retrievedSettings).toEqual(settings)
})
})
})
16 changes: 15 additions & 1 deletion packages/backend/src/tenants/settings/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export interface TenantSettingService {
pagination?: Pagination,
sortOrder?: SortOrder
) => Promise<TenantSetting[]>
getSettingsByPrefix: (prefix: string) => Promise<TenantSetting[]>
}

export interface ServiceDependencies extends BaseService {
Expand All @@ -68,7 +69,9 @@ export async function createTenantSettingService(
tenantId: string,
pagination?: Pagination,
sortOrder?: SortOrder
) => getTenantSettingPageForTenant(deps, tenantId, pagination, sortOrder)
) => getTenantSettingPageForTenant(deps, tenantId, pagination, sortOrder),
getSettingsByPrefix: (prefix: string) =>
getWalletAddressSettingsByPrefix(deps, prefix)
}
}

Expand Down Expand Up @@ -147,3 +150,14 @@ async function getTenantSettingPageForTenant(
.andWhere('tenantId', tenantId)
.getPage(pagination, sortOrder)
}

async function getWalletAddressSettingsByPrefix(
deps: ServiceDependencies,
prefix: string
): Promise<TenantSetting[]> {
return await TenantSetting.query(deps.knex)
.whereILike('value', `${prefix}%`)
.andWhere({
key: TenantSettingKeys.WALLET_ADDRESS_URL.name
})
}
Loading