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: add base parameter to /rates API #1527

Merged
merged 4 commits into from
Jun 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 23 additions & 5 deletions localenv/mock-account-servicing-entity/app/routes/rates.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,33 @@
import type { LoaderArgs } from '@remix-run/node'
import { json } from '@remix-run/node'

interface Rates {
[currency: string]: { [currency: string]: number }
}

const exchangeRates: Rates = {
USD: {
EUR: 1.1602,
ZAR: 17.3792
},
EUR: {
USD: 0.8619,
ZAR: 20.44
},
ZAR: {
USD: 0.0575,
EUR: 0.0489
}
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
export function loader({ request }: LoaderArgs) {
const base = new URL(request.url).searchParams.get('base') || 'USD'

return json(
{
base: 'USD',
rates: {
EUR: 1.1602,
ZAR: 17.3792
}
base,
rates: exchangeRates[base] ?? {}
},
{ status: 200 }
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ describe('OutgoingPaymentService', (): void => {
Config.exchangeRatesUrl = 'https://test.rates'
nock(Config.exchangeRatesUrl)
.get('/')
.query(true)
.reply(200, () => ({
base: 'USD',
rates: {
Expand Down
1 change: 1 addition & 0 deletions packages/backend/src/open_payments/quote/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ describe('QuoteService', (): void => {
Config.signatureSecret = SIGNATURE_SECRET
nock(Config.exchangeRatesUrl)
.get('/')
.query(true)
.reply(200, () => ({
base: 'USD',
rates: {
Expand Down
8 changes: 5 additions & 3 deletions packages/backend/src/open_payments/quote/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,11 @@ export async function startQuote(
deps: ServiceDependencies,
options: StartQuoteOptions
): Promise<Pay.Quote> {
const rates = await deps.ratesService.rates().catch((_err: Error) => {
throw new Error('missing rates')
})
const rates = await deps.ratesService
.rates(options.receiver.assetCode)
.catch((_err: Error) => {
throw new Error('missing rates')
})

const plugin = deps.makeIlpPlugin({
sourceAccount: options.paymentPointer,
Expand Down
9 changes: 8 additions & 1 deletion packages/backend/src/openapi/exchange-rates.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,14 @@ tags:
description: Exchange rates
paths:
/:
parameters: []
parameters:
- schema:
type: string
minLength: 1
name: base
in: query
required: true
description: Base exchange rate Base exchange rate
get:
summary: Fetch exchange rates
operationId: get-rates
Expand Down
26 changes: 14 additions & 12 deletions packages/backend/src/rates/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { convert, ConvertOptions } from './util'
const REQUEST_TIMEOUT = 5_000 // millseconds

export interface RatesService {
rates(): Promise<Rates>
rates(baseAssetCode: string): Promise<Rates>
convert(
opts: Omit<ConvertOptions, 'exchangeRate'>
): Promise<bigint | ConvertError>
Expand Down Expand Up @@ -59,7 +59,7 @@ class RatesServiceImpl implements RatesService {
if (sameCode && sameScale) return opts.sourceAmount
if (sameCode) return convert({ exchangeRate: 1.0, ...opts })

const rates = await this.sharedLoad()
const rates = await this.sharedLoad(opts.sourceAsset.code)
const sourcePrice = rates[opts.sourceAsset.code]
if (!sourcePrice) return ConvertError.MissingSourceAsset
if (!isValidPrice(sourcePrice)) return ConvertError.InvalidSourcePrice
Expand All @@ -73,11 +73,11 @@ class RatesServiceImpl implements RatesService {
return convert({ exchangeRate, ...opts })
}

async rates(): Promise<Rates> {
return this.sharedLoad()
async rates(baseAssetCode: string): Promise<Rates> {
return this.sharedLoad(baseAssetCode)
}

private sharedLoad(): Promise<Rates> {
private sharedLoad(baseAssetCode: string): Promise<Rates> {
if (this.ratesRequest && this.ratesExpiry) {
if (this.ratesExpiry < new Date()) {
// Already expired: invalidate cached rates.
Expand All @@ -89,7 +89,7 @@ class RatesServiceImpl implements RatesService {
) {
// Expiring soon: start prefetch.
if (!this.prefetchRequest) {
this.prefetchRequest = this.loadNow().finally(() => {
this.prefetchRequest = this.loadNow(baseAssetCode).finally(() => {
this.prefetchRequest = undefined
})
}
Expand All @@ -99,7 +99,7 @@ class RatesServiceImpl implements RatesService {
if (!this.ratesRequest) {
this.ratesRequest =
this.prefetchRequest ||
this.loadNow().catch((err) => {
this.loadNow(baseAssetCode).catch((err) => {
this.ratesRequest = undefined
this.ratesExpiry = undefined
throw err
Expand All @@ -108,14 +108,16 @@ class RatesServiceImpl implements RatesService {
return this.ratesRequest
}

private async loadNow(): Promise<Rates> {
private async loadNow(baseAssetCode: string): Promise<Rates> {
const url = this.deps.exchangeRatesUrl
if (!url) return {}

const res = await this.axios.get<RatesResponse>(url).catch((err) => {
this.deps.logger.warn({ err: err.message }, 'price request error')
throw err
})
const res = await this.axios
.get<RatesResponse>(url, { params: { base: baseAssetCode } })
.catch((err) => {
this.deps.logger.warn({ err: err.message }, 'price request error')
throw err
})

const { base, rates } = res.data
this.checkBaseAsset(base)
Expand Down
106 changes: 55 additions & 51 deletions packages/backend/src/rates/util.test.ts
Original file line number Diff line number Diff line change
@@ -1,60 +1,64 @@
import { convert, Asset } from './util'

describe('Rates util', function () {
describe('convert', function () {
it('converts same scales', () => {
// Rate > 1
expect(
convert({
exchangeRate: 1.5,
sourceAmount: 100n,
sourceAsset: asset(9),
destinationAsset: asset(9)
})
).toBe(150n)
// Rate < 1
expect(
convert({
exchangeRate: 0.5,
sourceAmount: 100n,
sourceAsset: asset(9),
destinationAsset: asset(9)
})
).toBe(50n)
// Round down
expect(
convert({
exchangeRate: 0.5,
sourceAmount: 101n,
sourceAsset: asset(9),
destinationAsset: asset(9)
})
).toBe(50n)
Comment on lines -25 to -32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

describe('Rates util', () => {
describe('convert', () => {
describe('convert same scales', () => {
test.each`
exchangeRate | sourceAmount | assetScale | expectedResult | description
${1.5} | ${100n} | ${9} | ${150n} | ${'exchange rate above 1'}
${1.1602} | ${12345n} | ${2} | ${14323n} | ${'exchange rate above 1 with rounding up'}
${1.1602} | ${10001n} | ${2} | ${11603n} | ${'exchange rate above 1 with rounding down'}
${0.5} | ${100n} | ${9} | ${50n} | ${'exchange rate below 1'}
${0.5} | ${101n} | ${9} | ${51n} | ${'exchange rate below 1 with rounding up'}
${0.8611} | ${1000n} | ${2} | ${861n} | ${'exchange rate below 1 with rounding down'}
`(
'$description',
async ({
exchangeRate,
sourceAmount,
assetScale,
expectedResult
}): Promise<void> => {
expect(
convert({
exchangeRate,
sourceAmount,
sourceAsset: createAsset(assetScale),
destinationAsset: createAsset(assetScale)
})
).toBe(expectedResult)
}
)
})

it('converts different scales', () => {
// Scale low → high
expect(
convert({
exchangeRate: 1.5,
sourceAmount: 100n,
sourceAsset: asset(9),
destinationAsset: asset(12)
})
).toBe(150_000n)
// Scale high → low
expect(
convert({
exchangeRate: 1.5,
sourceAmount: 100_000n,
sourceAsset: asset(12),
destinationAsset: asset(9)
})
).toBe(150n)
describe('convert different scales', () => {
test.each`
exchangeRate | sourceAmount | sourceAssetScale | destinationAssetScale | expectedResult | description
${1.5} | ${100n} | ${9} | ${12} | ${150_000n} | ${'convert scale from low to high'}
${1.5} | ${100_000n} | ${12} | ${9} | ${150n} | ${'convert scale from high to low'}
`(
'$description',
async ({
exchangeRate,
sourceAmount,
sourceAssetScale,
destinationAssetScale,
expectedResult
}): Promise<void> => {
expect(
convert({
exchangeRate,
sourceAmount,
sourceAsset: createAsset(sourceAssetScale),
destinationAsset: createAsset(destinationAssetScale)
})
).toBe(expectedResult)
}
)
})
})
})

function asset(scale: number): Asset {
return { code: '[ignored]', scale }
function createAsset(scale: number): Asset {
return { code: 'XYZ', scale }
}
7 changes: 2 additions & 5 deletions packages/backend/src/rates/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,8 @@ export interface Asset {
}

export function convert(opts: ConvertOptions): bigint {
const maxScale = Math.max(opts.sourceAsset.scale, opts.destinationAsset.scale)
const shiftUp = 10 ** maxScale
const scaleDiff = opts.destinationAsset.scale - opts.sourceAsset.scale
const scaledExchangeRate = opts.exchangeRate * 10 ** scaleDiff
return (
(opts.sourceAmount * BigInt(scaledExchangeRate * shiftUp)) / BigInt(shiftUp)
)

return BigInt(Math.round(Number(opts.sourceAmount) * scaledExchangeRate))
Copy link
Contributor Author

@mkurapov mkurapov Jun 28, 2023

Choose a reason for hiding this comment

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

BigInt divided by BigInt ends up truncating and not rounding the result. I think for currency, rounding up or down to the nearest unit makes more sense (and is standard). We can't really get around using number, since we are dealing with decimals for the exchange rate

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to be concerned about exceeding the MAX_SAFE_INTEGER (in the Number(opts.sourceAmount) case)?

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'm quite certain we will never hit payment amounts over 9007199254740991 in any realistic scenario, but I can add a log just to be extra safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have any tooling right now for proper alerting/error management, and I feel like a log would just get swallowed. Going to merge this in for now.

}