Currency: Common currency reference conversion#4205
Conversation
Closes prebid#3566 The Prebid.js currency module is able to cross convert currencies using USD (or GBP) as an intermediate. This feature is not currently implemented in PBS Go, so the following error exists: 'Unable to convert from currency NOK to desired ad server currency SEK'. In this PR we implement the cross currency conversion logic through an intermediate currency as a part of the GetRate method in the currency package.
guscarreon
left a comment
guscarreon
left a comment
There was a problem hiding this comment.
Take the conversion rates shown below:
rates := NewRates(map[string]map[string]float64{
"USD": {
"CAD": 0.90,
"MXN": 20.00,
"JPY": 50.00,
},
"MXN": {
"EUR": 0.05,
},
})
MAybe it's just me but, from "intermediate conversion" I would think that I'd be able to convert USD to EUR via MXN. In other words, convert from USD to MXN first, followed by a second conversion from MXN to EUR (USD -> MXN -> EUR) but, with the logic implemented in this pull request, that's not the case, right? The implemented logic would help me say, convert from Canadian Dollars to Japanese Yens (CAD -> JPY) that are both found under USD.
- Do you intend to implement
USD->MXN->EURconversions too? - If not, should we rename this type of conversion to "common currency reference conversion" or something of sorts? Or do you agree with the naming?
currency/rates.go
Outdated
| if fromPresent { | ||
| return toConversion / fromConversion, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we move lines 51 to 60 to their own function and code a table-driven test in currency/rates_test.go?
@guscarreon our intention was to fulfil the feature #3566 where it has been mentioned as an intermediate currency. Moreover, we have investigated the implementation in PBS Java and Prebid.js and based on the findings we wanted to be as close to "spec" as possible in order not to run into inconsistency between implementations - Go and Java versions being nearly identical and Prebid.js with minimal differences. However, I agree that common currency reference conversion name would fit better. Also, I guess if we want to make more functional implementation it would be much better to extend conversion tables either to n*n currency mapping to minimize runtime efforts (plain lookup instead of looping) or make all the logic via one explicit common currency, but then this issue would have a bit wider scope and all 3 implementation between different Prebid versions would have different logic implemented. What do you think? |
@sigma-software-prebid thank you for the clarification. I agree with catching up to PBS-Java and Prebid.js. One final question: can we, as stated in a comment on my previous review above, move lines 51 to 60 in |
… the review feedback
sigma-software-prebid
changed the title
@guscarreon thank you for the feedback. We have extracted the above-mentioned code into the separate |
guscarreon
left a comment
guscarreon
left a comment
There was a problem hiding this comment.
@sigma-software-prebid thank you for addressing my feedback. LGTM
currency/rates_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func TestGetRate_FindConversionRate(t *testing.T) { |
There was a problem hiding this comment.
The test cases are on point. I also appreciate you following the approach and style used in this file. However, I suggest deviating from this and instead using a more current approach that follows best practices and our preferred style (which I still need to document) for all unit tests going forward. This involves:
- putting the description first in the struct
- using
t.Run - keeping descriptions short and separating the words with underscores which
t.Rundoes automatically so that if a test fails we can simply copy the description from the output and search for it in the code - avoiding adding extra descriptions at the end of the
assertstatements sincet.Runwill assist in making it clear what failed - removing unnecessary comments
- tightening up the white space
func TestGetRate_FindConversionRate(t *testing.T) {
rates := NewRates(map[string]map[string]float64{
"USD": {
"SEK": 10.23842,
"NOK": 10.47089,
},
"EUR": {
"THB": 35.23842,
"ZAR": 18.47089,
},
})
testCases := []struct {
description string
from string
to string
expectedRate float64
hasError bool
}{
{
description: "in_same_intermediate_USD_currency",
from: "NOK",
to: "SEK",
expectedRate: 0.9777984488424574,
},
{
description: "in_same_intermediate_USD_currency_inverse",
from: "SEK",
to: "NOK",
expectedRate: 1 / 0.9777984488424574,
},
{
description: "in_same_intermediate_EUR_currency",
from: "THB",
to: "ZAR",
expectedRate: 0.5241690745498806,
},
{
description: "in_same_intermediate_EUR_currency_inverse",
from: "ZAR",
to: "THB",
expectedRate: 1 / 0.5241690745498806,
},
{
description: "in_different_intermediate_currencies",
from: "NOK",
to: "ZAR",
expectedRate: 0,
hasError: true,
},
}
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
fromUnit, err := currency.ParseISO(tc.from)
if err != nil {
t.Errorf("Expected no error, but got: %v", err)
}
toUnit, err := currency.ParseISO(tc.to)
if err != nil {
t.Errorf("Expected no error, but got: %v", err)
}
rate, err := FindConversionRate(rates, fromUnit, toUnit)
if tc.hasError {
assert.NotNil(t, err)
assert.Equal(t, float64(0), rate)
} else {
assert.Nil(t, err)
assert.Equal(t, tc.expectedRate, rate)
}
})
}
}
There was a problem hiding this comment.
@bsardo thank you for the review. We have addressed all your feedback. Please, let us know what do you think.
Rename FindConversionRate to FindIntermediateConversionRate and adjust test structure and formating to the new guidelines
4 participants
🔧 Type of changes
✨ What's the context?
#3566
🧠 Rationale behind the change
The Prebid.js currency module is able to cross convert currencies using USD (or GBP) as a common currency reference. This feature is not currently implemented in PBS Go, so the following error exists:
Unable to convert from currency NOK to desired ad server currency SEK.In this PR we implement the common currency reference conversion logic (
FindIntermediateConversionRatemethod) in thecurrencypackage.Related change in PBS Java
🧪 Test plan
New functionality is covered by
TestGetRate_FindIntermediateConversionRateunit test incurrency/rates_test.go.