-
Notifications
You must be signed in to change notification settings - Fork 295
Allow for a currency unit in fee:check responses #2922
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
Conversation
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @gbrodman).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 393 at r1 (raw file):
ImmutableSet<CurrencyUnit> seenCurrencies = currenciesBuilder.build(); CurrencyUnit currency = seenCurrencies.size() == 1 ? seenCurrencies.iterator().next() : CurrencyUnit.USD;
Is defaulting to USD the correct thing to do? Do we set a default registry currency anywhere? What if seenCurrencies contained two currencies here, neither of which was USD? Then defaulting to USD would just flat out be wrong?
And more broadly speaking, what is the logic here? Incoming domain checks can be in multiple currencies? And when that happens, how exactly do we handle it in the response?
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @gbrodman).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 393 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Is defaulting to USD the correct thing to do? Do we set a default registry currency anywhere? What if
seenCurrenciescontained two currencies here, neither of which was USD? Then defaulting to USD would just flat out be wrong?And more broadly speaking, what is the logic here? Incoming domain checks can be in multiple currencies? And when that happens, how exactly do we handle it in the response?
Honestly it sounds like we should just ignore this incoming parameter, as Nomulus only supports one currency per TLD and it doesn't matter what they ask for, they're going to get what the currency actually is?
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @gbrodman).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 393 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Honestly it sounds like we should just ignore this incoming parameter, as Nomulus only supports one currency per TLD and it doesn't matter what they ask for, they're going to get what the currency actually is?
(And we're not going to be doing live conversion based on current exchange rates.)
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbrodman made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @CydeWeys).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 393 at r1 (raw file):
Incoming domain checks can be in multiple currencies? And when that happens, how exactly do we handle it in the response?
Yup, this is what I noted in chat yesterday -- the RFC doesn't seem to mention or handle this case :(
I'll just add a config field in the "misc" section, "defaultCurrency"
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbrodman made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @CydeWeys).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 393 at r1 (raw file):
Previously, gbrodman wrote…
Incoming domain checks can be in multiple currencies? And when that happens, how exactly do we handle it in the response?
Yup, this is what I noted in chat yesterday -- the RFC doesn't seem to mention or handle this case :(
I'll just add a config field in the "misc" section, "defaultCurrency"
or, "defaultCurrencyInDomainChecks"
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbrodman made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @CydeWeys).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 393 at r1 (raw file):
Previously, gbrodman wrote…
or, "defaultCurrencyInDomainChecks"
actually maybe we should just throw an exception if you try to issue checks for domains across TLDs with different currencies
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @gbrodman).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 393 at r1 (raw file):
Previously, gbrodman wrote…
actually maybe we should just throw an exception if you try to issue checks for domains across TLDs with different currencies
ACK that is this OK for now to unblock RST, but we will revisit after hearing back from registrars on what behavior they typically expect from other registries.
Also, are any registrars currently performing domain:checks on .minna domains as well as other USD-denominated ones? If so this is going to cause those commands to fail, so we should at least go into it knowing what's going to happen.
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman).
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbrodman made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CydeWeys).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 393 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
ACK that is this OK for now to unblock RST, but we will revisit after hearing back from registrars on what behavior they typically expect from other registries.
Also, are any registrars currently performing
domain:checks on .minna domains as well as other USD-denominated ones? If so this is going to cause those commands to fail, so we should at least go into it knowing what's going to happen.
over one 24 hour period, there were approximately 1230 such requests, 1146 of which came from porkbun. It seems like all the porkbun ones used the fee extension, not sure about the others
select * from (SELECT JSON_EXTRACT_ARRAY(jsonValue, '$.targetIds') as targetIds, JSON_EXTRACT(jsonValue, '$.serverTrid') as serverTrid, JSON_EXTRACT(jsonValue, '$.clientId') as registrarId FROM(
SELECT substr(jsonPayload.message, 30) as jsonValue FROM `domain-registry.gke_logs.stderr_20260107`
WHERE starts_with(jsonPayload.message, 'FLOW-LOG-SIGNATURE-METADATA')
and contains_substr(jsonPayload.message, '.xn--q9jyb4c')
and contains_substr(jsonPayload.message, 'srs-dom-check'))) where array_length(targetIds) > 1
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbrodman made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CydeWeys).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 393 at r1 (raw file):
Previously, gbrodman wrote…
over one 24 hour period, there were approximately 1230 such requests, 1146 of which came from porkbun. It seems like all the porkbun ones used the fee extension, not sure about the others
select * from (SELECT JSON_EXTRACT_ARRAY(jsonValue, '$.targetIds') as targetIds, JSON_EXTRACT(jsonValue, '$.serverTrid') as serverTrid, JSON_EXTRACT(jsonValue, '$.clientId') as registrarId FROM( SELECT substr(jsonPayload.message, 30) as jsonValue FROM `domain-registry.gke_logs.stderr_20260107` WHERE starts_with(jsonPayload.message, 'FLOW-LOG-SIGNATURE-METADATA') and contains_substr(jsonPayload.message, '.xn--q9jyb4c') and contains_substr(jsonPayload.message, 'srs-dom-check'))) where array_length(targetIds) > 1
looks like all those are on the old fee extension v0.6 so we can pass those through without issue
This is / will be required in https://datatracker.ietf.org/doc/rfc8748/. I split this out from the rest of the fee-extension testing so that it can be easily visible.
This is / will be required in https://datatracker.ietf.org/doc/rfc8748/ (section 3.2). I split this out from the rest of the fee-extension testing so that it can be easily visible.
This change is