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

Poor error handling when loading currency rates #10560

Closed
dgirardi opened this issue Oct 2, 2023 · 1 comment · Fixed by #10679
Closed

Poor error handling when loading currency rates #10560

dgirardi opened this issue Oct 2, 2023 · 1 comment · Fixed by #10679
Labels

Comments

@dgirardi
Copy link
Collaborator

dgirardi commented Oct 2, 2023

Type of issue

Bug

Description

If the currency module is set up to load rates from the network (the default) and no default rates are specified, bid responses are put on hold until rates are available. This is a problem when the file does not load quickly enough or the request fails:

  1. if rates are not available by the time an auction times out, bids are lost without necessarily causing an AUCTION_TIMEOUT event. Currently they'll generate NO_BID events instead; with Core & currency module: fix bug where bid responses are collected after auction ends #10558 they'll generate no events.
  2. the same happens if rates fail to load - all auctions will wait until the timeout and then end without any bids.

I think it makes sense to address #2 by trying to fetch rates again at the start of the next auction, but I am unsure about #1. If an auction times out and we have pending bids but no rates, should we:

  • reject the bids?
  • emit AUCTION_TIMEOUT without any timed out bidders?
  • postpone the timeout to wait for rates?
@dgirardi
Copy link
Collaborator Author

dgirardi commented Oct 3, 2023

Consensus is:

  • bids should be rejected with a currency-specific reason
  • bids that do not actually need a conversion (currency is already the adserver currency) should not wait for rates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging a pull request may close this issue.

1 participant