Skip to content

Enforce nonnegative costs for TLDs#3139

Merged
gbrodman merged 1 commit into
google:masterfrom
gbrodman:tldBuilder
Jul 10, 2026
Merged

Enforce nonnegative costs for TLDs#3139
gbrodman merged 1 commit into
google:masterfrom
gbrodman:tldBuilder

Conversation

@gbrodman

@gbrodman gbrodman commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

This change is Reviewable

@CydeWeys CydeWeys left a comment

Copy link
Copy Markdown
Member

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 6 files reviewed, 1 unresolved discussion (waiting on gbrodman).


core/src/main/java/google/registry/tools/ConfigureTldCommand.java line 168 at r1 (raw file):

    }
    // Enforce any builder restrictions, e.g. "no negative fees"
    newTld = newTld.asBuilder().build();

This is weird. It makes more sense to me to move all the verification to another method that you call here, and then call it from within the build() method as well.

@gbrodman gbrodman left a comment

Copy link
Copy Markdown
Collaborator Author

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 6 files reviewed, 1 unresolved discussion (waiting on CydeWeys).


core/src/main/java/google/registry/tools/ConfigureTldCommand.java line 168 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This is weird. It makes more sense to me to move all the verification to another method that you call here, and then call it from within the build() method as well.

Done.

@CydeWeys CydeWeys left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CydeWeys made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on gbrodman).


core/src/main/java/google/registry/model/tld/Tld.java line 817 at r2 (raw file):

  }

  public void validateState() {

Worth adding a short Javadoc here, as this is a public method. (And also this is different from other ImmutableObjects.)

Note: we remove the checks in the setters because we check them all in
the build method instead (and the setters are only called in tests).

@gbrodman gbrodman left a comment

Copy link
Copy Markdown
Collaborator Author

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 6 files reviewed, 1 unresolved discussion (waiting on CydeWeys).


core/src/main/java/google/registry/model/tld/Tld.java line 817 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Worth adding a short Javadoc here, as this is a public method. (And also this is different from other ImmutableObjects.)

Done.

@CydeWeys CydeWeys left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CydeWeys resolved 1 discussion.
Reviewable status: 0 of 6 files reviewed, all discussions resolved.

@CydeWeys CydeWeys left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CydeWeys reviewed 6 files and all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on gbrodman).

@gbrodman gbrodman added this pull request to the merge queue Jul 10, 2026
Merged via the queue into google:master with commit b8a51ca Jul 10, 2026
16 checks passed
@gbrodman gbrodman deleted the tldBuilder branch July 10, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants