Currency support #280 - step 1 #634

benjaminch · 2018-07-29T15:39:09Z

This CL allows prebid server to reject any bids having a currency mismatch between allowed
currencies in the bid request and the currency declared in bids.

For the time being, if allowed currencies are not declared in bid request or in bid
the currency is implicitly set to USD to prevent from any breaking changes.

This CL includes:

Prebid server to reject bids having currency mistmatch

Tnis CL doesn't include:

Changing the default currency (which is USD)

More details in #280

dbemiller

Looking good... but I do see a few issues.

dbemiller · 2018-07-30T12:41:35Z

exchange/bidder.go

@@ -119,6 +123,7 @@ func (bidder *bidderAdapter) requestBid(ctx context.Context, request *openrtb.Bi
 			bidResponse, moreErrs := bidder.Bidder.MakeBids(request, httpInfo.request, httpInfo.response)
 			errs = append(errs, moreErrs...)
 			if bidResponse != nil {
+				seatBid.currency = bidResponse.Currency


Two concerns here:

Looks like this should have a != "" check. Right now, it looks like it will return an empty string for a default because you'll override the USD you set earlier, even if bidders don't define it. There should probably be a unit test for this too.

This assumes that every HTTP request uses the same currency... which isn't necessarily true.

It's probably not very likely... but it would be really bad if publishers got false bids.

I think we should take the currency from their first response, and then ignore the bids & return errors if future responses don't match that one. In the future, we can use the currency conversion rates to translate them. (probably should leave a TODO about this so we don't forget it, too)

Ok for 1.
For 2., I am not sure how you want to achieve this. Do you prefer having the currency at bid level directly ? It will avoid the case you are describing right?

ah, sorry. No. We deliberately chose not to support per-Bid currencies in the ticket, and I think that was the right call.

This only affects Bidders who make several HTTP calls for each BidRequest. Although the types on Bidder make sure that each HTTP call has the same currency, Prebid can't make sure that each HTTP call returns bids with the same currency as the others.

The question here is: "if a Bidder makes two HTTP calls, and their servers respond with different currencies, what do we do?"

Mine answer would be: first try to convert them using the currency conversion rates, and then ignore the ones we can't convert and return an error to help publishers debug. Since conversion rates aren't implemented yet, the best we can do right now is return the errors if they don't match. No major structural changes... just some logic inside this function so that you use the currency from their first response as the source of truth, and then reject others which contradict it.

Let me know if you have any better ideas though...

Ok, so, if we don't take into consideration the exchange rate mechanism for the time being, what you are proposing is to check bid response currency against bid request allowed currencies.
The first approach was to assume that all HTTP calls will end up having the same currency but it's not the case.
I am not sure to understand how it would happen though, how would bidders set differents currencies accross several HTTP calls ? Especially, why wouldn't it be rejected already with the current implementation (sorry but I really want to make sure I fully understand the technical details here :)). Thanks

Suppose a publisher makes a BidRequest like:

{ "cur": ["USD", "EUR"], "imp": [ "banner": { ... }, "video": { ... } ] }

The Bidder's servers don't support multiformat, so they split it into two separate HTTP requests:

{ "cur": ["USD", "EUR"], "imp": [ "banner": { ... }, ] }

{ "cur": ["USD", "EUR"], "imp": [ "video": { ... } ] }

According to OpenRTB, either currency is allowed. Based on their inventory, their servers return these two responses:

{ "cur": "USD", "seatbid": { ... some Banner bids nested in here ... } }

{ "cur": "EUR", "seatbid": { ... some Video bids nested in here ... } }

The adapter's MakeRequests function gets called twice--once with each ResponseData. One call returns bids in USD, and the second returns bids in EUR.

This code is responsible for merging the bids from the two calls. In pseudocode, yours does:

seatBid = { currency: "USD", bids: [] } for each response { seatBid.currency = response.currency seatBid.bids = append(seatBid.bids, response.bids) } return seatBid

But... this isn't really true. The Bidder will think they made some bids in EUR, and others in USD. As written, PBS will report all those bids as being EUR, without doing any conversions for them.

Does that make sense? If not, could you point me to the place where you think this is being rejected?

Oh, I see, ok it does make sense to me !
I will update the code accordingly then.

Thanks

dbemiller · 2018-07-30T12:53:17Z

exchange/exchange.go

+
+	// TODO: #280 get default currency from config.
+	// For the time being, default currency is USD.
+	if _, cerr := validateCurrency(request.Cur, brw.adapterBids.currency, "USD"); cerr != nil {


Do you have any use case for a configurable default currency?

I'm wary of how confusing it could be to publishers if they change PBS hosts and suddenly get wildly different bids... my preference would be to just hardcode the USD default and be done with it.

Not really, it's just that I am used to avoid any hardcoded values just in case, but I agree that on this one, we probably don't want to be flexible.

dbemiller · 2018-07-30T12:59:41Z

exchange/exchange.go

+	// For the time being, default currency is USD.
+	if _, cerr := validateCurrency(request.Cur, brw.adapterBids.currency, "USD"); cerr != nil {
+		err = append(err, cerr)
+		return


You'll also need to set brw.adapterbids.bids = nil here before returning, because all the bids are invalid if the currency is wrong.

Good catch !

hhhjort · 2018-07-30T14:53:27Z

exchange/exchange.go

@@ -402,6 +410,40 @@ func (brw *bidResponseWrapper) validateBids() (err []error) {
 	return err
 }

+// validateCurrency will run currency validation checks and return true if it passes, false otherwise.
+func validateCurrency(requestAllowedCurrencies []string, bidCurrency string, defaultCurrency string) (bool, error) {


Do we want to return both a boolean and an error? Currently we don't read the boolean value anywhere. If error is non-nil, we always return false, so I don't see what the boolean flag might gain us in future use.

No, I agree we don't need this boolean, I'll remove it.

hhhjort · 2018-07-30T14:58:09Z

exchange/exchange.go

+		// If bid currency is not set, then consider it's default currency.
+		bidCurrency = defaultCurrency
+	}
+	currency, cerr := currency.ParseISO(bidCurrency)


This might be a little fragile. Here we are redefining currency from being a reference to the currency module to be a Unit from that module. I am a little surprised that go did not complain about currency being the wrong type here actually. In any case, it may lead to confusion having multiple definitions of currency in the same function. I would rename the variable to cur or currencyUnit.

Good point, thanks !

hhhjort

My addition suggestions are not as critical as the issues dbemiller already raised, but the PR does need some updating.

benjaminch · 2018-08-09T18:28:08Z

Hello @dbemiller, @hhhjort,

Just pushed an update taking into account your comments.
Let me know if you think that the test coverage is ok or not or if anything else need to be improved :)

Thanks !

hhhjort

validateCurrency(requestAllowedCurrencies []string, bidCurrency string, defaultCurrency string) still has the option for a different default currency, but it is hardcoded to "USD" at the calling point, so I suppose it makes it marginally easier to make an option later if needed. Still it would probably be better to remove it, as it is easy enough to add back in if we find we do need it. Not a big enough issue to be fatal for the PR, but not having it would make it clearer that it currently is not an option for someone reading the code.

benjaminch · 2018-08-09T19:14:56Z

Hi @hhhjort,

Sorry my bad I forgot this one, i’ll Remove the option and hardcore it !

This CL allows prebid server to reject any bids having a currency mismatch between allowed currencies in the bid request and the currency declared in bids. For the time being, if allowed currencies are not declared in bid request or in bid the currency is implicitly set to `USD` to prevent from any breaking changes. This CL includes: - Prebid server to reject bids having currency mistmatch Tnis CL doesn't include: - Changing the default currency (which is USD)

benjaminch · 2018-08-10T08:47:15Z

Just removed the option to pass default currency @hhhjort, thanks for the feedback :)

Changes have been made

dbemiller

looks good to me too!

This CL allows prebid server to reject any bids having a currency mismatch between allowed currencies in the bid request and the currency declared in bids. For the time being, if allowed currencies are not declared in bid request or in bid the currency is implicitly set to `USD` to prevent from any breaking changes. This CL includes: - Prebid server to reject bids having currency mistmatch Tnis CL doesn't include: - Changing the default currency (which is USD)

dbemiller previously requested changes Jul 30, 2018

View reviewed changes

dbemiller requested a review from hhhjort July 30, 2018 13:06

dbemiller assigned dbemiller and hhhjort Jul 30, 2018

hhhjort reviewed Jul 30, 2018

View reviewed changes

hhhjort requested changes Jul 30, 2018

View reviewed changes

benjaminch force-pushed the prevent-bidding-with-wrong-currency branch from 78fef45 to 1eae22a Compare August 9, 2018 18:26

hhhjort previously approved these changes Aug 9, 2018

View reviewed changes

benjaminch dismissed hhhjort’s stale review via 0f8774d August 10, 2018 08:45

benjaminch force-pushed the prevent-bidding-with-wrong-currency branch from 1eae22a to 0f8774d Compare August 10, 2018 08:45

hhhjort approved these changes Aug 10, 2018

View reviewed changes

dbemiller approved these changes Aug 10, 2018

View reviewed changes

dbemiller merged commit 56ce31f into prebid:master Aug 10, 2018

benjaminch mentioned this pull request Mar 8, 2019

Open RTB: Add currency support #280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Currency support #280 - step 1 #634

Currency support #280 - step 1 #634

benjaminch commented Jul 29, 2018

dbemiller left a comment

dbemiller Jul 30, 2018

benjaminch Jul 30, 2018

dbemiller Jul 30, 2018 •

edited

Loading

benjaminch Jul 30, 2018

dbemiller Jul 30, 2018

benjaminch Jul 30, 2018

dbemiller Jul 30, 2018

benjaminch Jul 30, 2018

dbemiller Jul 30, 2018

benjaminch Jul 30, 2018

hhhjort Jul 30, 2018

benjaminch Jul 30, 2018

hhhjort Jul 30, 2018

benjaminch Jul 30, 2018

hhhjort left a comment

benjaminch commented Aug 9, 2018

hhhjort left a comment

benjaminch commented Aug 9, 2018

benjaminch commented Aug 10, 2018

dbemiller left a comment

Currency support #280 - step 1 #634

Currency support #280 - step 1 #634

Conversation

benjaminch commented Jul 29, 2018

dbemiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

benjaminch commented Aug 9, 2018

hhhjort left a comment

Choose a reason for hiding this comment

benjaminch commented Aug 9, 2018

benjaminch commented Aug 10, 2018

dbemiller left a comment

Choose a reason for hiding this comment

dbemiller Jul 30, 2018 •

edited

Loading