Implement Expiry Access Period flows and billing#3131
Conversation
080db94 to
d642fda
Compare
| .build(); | ||
| } | ||
|
|
||
| private static BillingEvent createXapBillingEvent( |
There was a problem hiding this comment.
This method is exactly the same as createEapBillingEvent() above it, except with a different reason and fee cost -- consolidate the two into a single helper that takes the fee reason and cost as parameters. It can be called something like createAdditionalBillingEvent() or similar.
There was a problem hiding this comment.
Done. Replaced both createEapBillingEvent and createXapBillingEvent with a unified createAdditionalBillingEvent(Reason reason, Money cost, BillingEvent createBillingEvent) helper method.
| feesBuilder.addFeeOrCredit(eapFee); | ||
| } | ||
|
|
||
| // Create the XAP fee, if any. |
There was a problem hiding this comment.
This method has now gotten very long; consider refactoring?
There was a problem hiding this comment.
Done. Extracted computeCreateFee (for base create fee and sunrise/token discounts) and maybeAddXapFee (for XAP fee evaluation and extension requirements) into dedicated private helper methods, significantly simplifying getCreatePrice.
| <fee:command name="create"> | ||
| <fee:period unit="y">1</fee:period> | ||
| <fee:fee description="create">13.00</fee:fee> | ||
| <fee:fee description="%FEE_DESCRIPTION%">110.00</fee:fee> |
There was a problem hiding this comment.
This XML testdata file is only being used once, so templating the description here isn't necessary, and indeed makes this less readable. Don't use a variable here, just have the actual expected raw XML output.
There was a problem hiding this comment.
Same for any other XML testdata files you added that are only used in one test; they don't need to use variables.
There was a problem hiding this comment.
Done. Removed templated variables (%FEE_VERSION% and %FEE_DESCRIPTION%) and replaced them with literal expected raw XML strings (epp:fee-1.0 and exact timestamp strings formatted without .0 seconds to match JAXB marshalling).
There was a problem hiding this comment.
Done. Removed all templated variables from all four single-use XML files (domain_check_fee_xap_response.xml, domain_create_premium_xap.xml, domain_create_response_premium_xap.xml, and domain_create_response_xap_fee.xml) and updated their test callers to use fixed static timestamps.
ba82679 to
992ed1c
Compare
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 43 files and all commit messages, and made 9 comments.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on CydeWeys).
core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java line 302 at r2 (raw file):
if (recentlyDeletedDomain.isPresent() && !Registrar.loadByRegistrarIdCached(registrarId).get().getExpiryAccessPeriodEnabled()) { throw new DomainReservedException(domainName.toString());
similar to DomainCheckFlow, it doesn't seem to me that "reserved" is the right terminology
core/src/test/java/google/registry/flows/domain/DomainCheckFlowTest.java line 2627 at r2 (raw file):
@Test void testCheck_agpDeletedRegularDomain_duringXap_returnsAvailable() throws Exception {
need a similar test for anchor tenant i think (maybe also in DomainPricingLogicTest)
core/src/main/java/google/registry/model/EntityYamlUtils.java line 370 at r2 (raw file):
public TimedTransitionProperty<ExpiryAccessPeriodMode> deserialize( JsonParser jp, DeserializationContext context) throws IOException { SortedMap<String, String> valueMap = jp.readValueAs(SortedMap.class);
Use a TypeReference here to avoid unchecked casts (we can do this in the other deserializers in this file too)
core/src/test/resources/google/registry/model/tld/tld.yaml line 35 at r2 (raw file):
amount: 0.00 escrowEnabled: false expiryAccessPeriodTransitions:
why do we (need to?) add this field in all of the existing TLDs? Isn't this the default value?
if we need to add these here, we'd need to do the same in the internal repo too right?
core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java line 1186 at r2 (raw file):
} Duration agpLength = tld.getAddGracePeriodLength(); if (tld.getAnchorTenantAddGracePeriodLength() != null
this null check is unnecessary (there's a default)
but in general this seems wrong? We need to know if the previously-deleted domain was an anchor tenant, right?
core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java line 99 at r2 (raw file):
String domainName, Instant dateTime, Optional<Domain> existingDomain,
i don't think "existingDomain" is the right term, given that by definition the domain does not exist anymore. "previousDomain" or "expiredDomain"?
core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java line 106 at r2 (raw file):
throws EppException { CurrencyUnit currency = tld.getCurrency(); BaseFee createFee =
This doesn't need to be a BaseFee (it cannot, we hope, be a Credit)
core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java line 175 at r2 (raw file):
CurrencyUnit currency, FeesAndCredits.Builder feesBuilder) { if (existingDomain.isPresent()
I think this check could be unified with the similar check in DomainCheckFlow's loadDomainIfInXap in some util function.
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 291 at r2 (raw file):
&& loadDomainIfInXap(domainName.toString(), now, domainExpiryAccessPeriodTotalLength) .isPresent()) { return Optional.of("Reserved");
I don't think "reserved" is precisely the right term here. "In expiry access program"?
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 291 at r2 (raw file):
Previously, gbrodman wrote…
I don't think "reserved" is precisely the right term here. "In expiry access program"?
As per the PR description:
- Enforces registrar opt-in in DomainCheckFlow: when an unallocated domain is in XAP and the querying registrar has not opted in, domain:check returns avail="0" with reason "Reserved".
One of the design goals of implementing this feature is so that zero changes are visible to registrars who don't opt-in to XAP (which is expected to be the majority of them). Thus, we want to use a status that they already understand, and Reserved is the only existing one that makes sense as In Use would require the domain to actually still exist (which it won't, if they run a domain:info or RDAP to see what's going on).
All that being said though, when we launched GlobalBlock, the reason that returns for avail="false" in EPP responses is "Blocked by a GlobalBlock service", and very few registrars participate in that, so perhaps we can relax this design goal. I'll have to think on it, and consult with business team.
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java line 99 at r2 (raw file):
Previously, gbrodman wrote…
i don't think "existingDomain" is the right term, given that by definition the domain does not exist anymore. "previousDomain" or "expiredDomain"?
I mean, it is an existing domain object in the system, but point taken, expiredDomain is somewhat clearer.
0c6c2d2 to
35866df
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: 41 of 43 files reviewed, 9 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java line 99 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
I mean, it is an existing domain object in the system, but point taken,
expiredDomainis somewhat clearer.
Actually, going with recentlyDeletedDomain, as domains don't really expire in our system.
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: 41 of 43 files reviewed, 9 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java line 1186 at r2 (raw file):
Previously, gbrodman wrote…
this null check is unnecessary (there's a default)
but in general this seems wrong? We need to know if the previously-deleted domain was an anchor tenant, right?
I decided it's best just not to handle anchor tenant domains especially at all. We will thus always use the length of the standard AGP in determining whether to run XAP, so anchor tenant domains will get an XAP after their deletion if they are deleted after more than five days.
Also, and this is not something I think we can easily do unfortunately, but I would love to get rid of the concept of a separate anchor tenant AGP entirely. It's not worth the extra complication for such a tiny edge case.
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: 41 of 43 files reviewed, 9 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 291 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
As per the PR description:
- Enforces registrar opt-in in DomainCheckFlow: when an unallocated domain is in XAP and the querying registrar has not opted in, domain:check returns avail="0" with reason "Reserved".
One of the design goals of implementing this feature is so that zero changes are visible to registrars who don't opt-in to XAP (which is expected to be the majority of them). Thus, we want to use a status that they already understand, and Reserved is the only existing one that makes sense as In Use would require the domain to actually still exist (which it won't, if they run a domain:info or RDAP to see what's going on).
All that being said though, when we launched GlobalBlock, the reason that returns for avail="false" in EPP responses is
"Blocked by a GlobalBlock service", and very few registrars participate in that, so perhaps we can relax this design goal. I'll have to think on it, and consult with business team.
Thinking more on this ... most new gTLD registries support GlobalBlock, but we'll be the only one doing XAP. So while it might make sense for most registries to do custom implementation for them, that doesn't necessarily translate to XAP.
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: 41 of 43 files reviewed, 9 unresolved discussions (waiting on gbrodman).
core/src/test/resources/google/registry/model/tld/tld.yaml line 35 at r2 (raw file):
Previously, gbrodman wrote…
why do we (need to?) add this field in all of the existing TLDs? Isn't this the default value?
if we need to add these here, we'd need to do the same in the internal repo too right?
We don't need it, as it is the default value, but we should add it, as the .yaml files are complete/exhaustive (listing all fields on the entities, not just the ones that happen to be different from whatever the current default is). Otherwise, there's always going to be diffs between the output of the get_tld nomulus command and what's checked into the repo, which will be confusing, and any new files generated for new TLDs will have these too.
And yes, I will subsequently add this to the internal repo as well (though because of the default, there's no dependency on ordering of doing so).
35866df to
2cec375
Compare
|
Addressing @gbrodman's review feedback from r2:
|
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 4 comments.
Reviewable status: 34 of 43 files reviewed, 9 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java line 106 at r2 (raw file):
Previously, gbrodman wrote…
This doesn't need to be a BaseFee (it cannot, we hope, be a Credit)
Done.
core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java line 175 at r2 (raw file):
Previously, gbrodman wrote…
I think this check could be unified with the similar check in DomainCheckFlow's loadDomainIfInXap in some util function.
Done.
core/src/main/java/google/registry/model/EntityYamlUtils.java line 370 at r2 (raw file):
Previously, gbrodman wrote…
Use a TypeReference here to avoid unchecked casts (we can do this in the other deserializers in this file too)
Done.
core/src/test/java/google/registry/flows/domain/DomainCheckFlowTest.java line 2627 at r2 (raw file):
Previously, gbrodman wrote…
need a similar test for anchor tenant i think (maybe also in DomainPricingLogicTest)
Done.
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 2 comments.
Reviewable status: 34 of 43 files reviewed, 9 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 291 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Thinking more on this ... most new gTLD registries support GlobalBlock, but we'll be the only one doing XAP. So while it might make sense for most registries to do custom implementation for them, that doesn't necessarily translate to XAP.
Let's table this for now, and if we decide to change it to something else based on Bruno/partner feedback prior to launch, we can do so? We still have several deployments left at minimum to change this before actually launching the feature, if we want to do so.
core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java line 302 at r2 (raw file):
Previously, gbrodman wrote…
similar to DomainCheckFlow, it doesn't seem to me that "reserved" is the right terminology
Ditto, although this one is less important as registrars tend to run the domain:check first, and they wouldn't then even attempt to send a domain:create when they know the domain has avail="false".
CydeWeys
left a comment
There was a problem hiding this comment.
PTAL.
@CydeWeys made 1 comment.
Reviewable status: 34 of 43 files reviewed, 9 unresolved discussions (waiting on gbrodman).
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 9 files and all commit messages, made 2 comments, and resolved 9 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on CydeWeys).
core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 291 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Let's table this for now, and if we decide to change it to something else based on Bruno/partner feedback prior to launch, we can do so? We still have several deployments left at minimum to change this before actually launching the feature, if we want to do so.
Yeah, I think that makes sense. Not the end of the world and I don't know if registrars do much caching (or would be confused if this name doesn't match the kinda-public list of reserved names) but eh if it's a big deal we can change it.
core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java line 1186 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
I decided it's best just not to handle anchor tenant domains especially at all. We will thus always use the length of the standard AGP in determining whether to run XAP, so anchor tenant domains will get an XAP after their deletion if they are deleted after more than five days.
Also, and this is not something I think we can easily do unfortunately, but I would love to get rid of the concept of a separate anchor tenant AGP entirely. It's not worth the extra complication for such a tiny edge case.
yeah this is all the right thing to do
2cec375 to
96fb7db
Compare
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 3 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on CydeWeys).
96fb7db to
6d7f9a3
Compare
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on CydeWeys).
6d7f9a3 to
88973cc
Compare
This commit implements the second stage (Java ORM mappings and EPP flow enforcement) of the Expiry Access Period (XAP) launch and opt-in mechanism, completing the Two-PR deployment split mandated by db/README.md after the Stage 1 database migrations (PR google#3134) are live. During XAP, a TLD charges a fee for domain registration during a timed period after deletion. To prevent accidental charges, registrars must explicitly opt in to participate in XAP registrations. Specifically, this commit: - Adds expiryAccessPeriodTransitions (a TimedTransitionProperty of ExpiryAccessPeriodMode) to Tld.java and ExpiryAccessPeriodModeTransitionUserType for mapping XAP mode schedules to PostgreSQL hstore columns. - Adds expiryAccessPeriodEnabled to Registrar.java with getter, builder setter, and JSON map serialization, and regenerates db-schema.sql.generated. - Enforces registrar opt-in in DomainCheckFlow: when an unallocated domain is in XAP and the querying registrar has not opted in, domain:check returns avail="0" with reason "Reserved". - Enforces registrar opt-in in DomainCreateFlow: when attempting to register a domain in XAP without registrar opt-in, throws DomainReservedException (EPP error 2304 "Object status prohibits operation"). - Enforces explicit fee acknowledgment during XAP domain creation when the XAP fee is non-zero, throwing FeesRequiredDuringExpiryAccessPeriodException in DomainFlowUtils if omitted. - Creates a one-time BillingEvent with Reason.FEE_EXPIRY_ACCESS when a domain is created during XAP with a non-zero fee, and adds getXapCost() to FeesAndCredits. - Updates all test TLD YAML configurations and adds comprehensive unit and integration tests across DomainCheckFlowTest, DomainCreateFlowTest, DomainPricingLogicTest, and RegistrarTest. TAG=agy CONV=f2488c74-8b4a-43f1-9c22-d1dddbdbb4e0 BUG=http://b/437398822
88973cc to
c5125fd
Compare
This commit implements the second stage (Java ORM mappings and EPP flow
enforcement) of the Expiry Access Period (XAP) launch and opt-in mechanism,
completing the Two-PR deployment split mandated by db/README.md after the
Stage 1 database migrations (PR #3134) are live.
During XAP, a TLD charges a fee for domain registration during a timed period
after deletion. To prevent accidental charges, registrars must explicitly
opt in to participate in XAP registrations.
Specifically, this commit:
ExpiryAccessPeriodMode) to Tld.java and ExpiryAccessPeriodModeTransitionUserType
for mapping XAP mode schedules to PostgreSQL hstore columns.
setter, and JSON map serialization, and regenerates db-schema.sql.generated.
in XAP and the querying registrar has not opted in, domain:check returns
avail="0" with reason "Reserved".
a domain in XAP without registrar opt-in, throws DomainReservedException
(EPP error 2304 "Object status prohibits operation").
fee is non-zero, throwing FeesRequiredDuringExpiryAccessPeriodException in
DomainFlowUtils if omitted.
is created during XAP with a non-zero fee, and adds getXapCost() to
FeesAndCredits.
integration tests across DomainCheckFlowTest, DomainCreateFlowTest,
DomainPricingLogicTest, and RegistrarTest.
TAG=agy
CONV=f2488c74-8b4a-43f1-9c22-d1dddbdbb4e0
BUG=http://b/437398822
This change is https://reviewable.io/reviews/google/nomulus/3131
This change is