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

proto: Refactor TokenDecodeError #2085

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

gretchenfrage
Copy link
Collaborator

@gretchenfrage gretchenfrage commented Dec 14, 2024

This PR consists solely of renames and docs changes.

  • Renames enum TokenDecodeError -> ValidationError

    TokenDecodeError is used when that there was an error decoding a token, but is also when there was no error decoding the token but the token contained an address not applicable to the current situation. Thus, this rename more accurately reflects the purpose of the error.

  • Renames variant UnknownToken -> Unusable Ignore

    UnknownToken is used when a token cannot be decoded or deserialized, and triggers the token to be ignored. Thus, it's name is currently an accurate description of how it's used, however, I found it very confusing why we use it like that, and had to get Ralith to explain it to me. Moreover, in the future, this error would be applicable to situations where a token can be unambiguously decrypted/decoded as a token from a NEW_TOKEN frame, but is simply not valid. In that situation, the variant name would cease to be an accurate description of how it's used. Thus, we rename it to a more accurately describe the sorts of situations it would apply to, and leave a detailed comment on the variant explaining the rationale. Relatedly, we remove a comment from endpoint.rs for being now-redundant.

    At one point, in Utilize NEW_TOKEN frames #1912, I tried the name InvalidMaybeNewToken, but Ralith seemed hesitant. While that name was more descriptive, it's also pretty cumbersome.

  • Renames variant WrongAddress -> InvalidRetry

    WrongAddress is used when a token can be successfully decoded and deserialized but has the wrong client address, and triggers the connection to be closed. Thus, it's name is currently an accurate description of how it's used. However, if in the future we start processing tokens from NEW_TOKEN frames, this error would not be applicable to situations where a token can be unambiguously decrypted/decoded as a token from a NEW_TOKEN frame but has the wrong client address, as the appropriate way to handle that situation would be to ignore the token rather than close the connection, so we would have to use a different error. In that situation, the variant name would cease to be an accurate description of how it's used. Thus, we rename it to a more accurately describe the sorts of situations it would apply to.

    An additional conceivable benefit of this name is that it would make it easier to potentially add future cases where a token can be unambiguously identified as a token from a Retry packet, but still cannot be fully decrypted or deserialized, or where it is otherwise for some reason unusable.

quinn-proto/src/endpoint.rs Outdated Show resolved Hide resolved
quinn-proto/src/token.rs Outdated Show resolved Hide resolved
@gretchenfrage gretchenfrage force-pushed the refactor-token-error branch 2 times, most recently from eb5423b to f53f8f2 Compare December 15, 2024 07:02
@djc
Copy link
Member

djc commented Dec 16, 2024

Nice!

@djc djc added this pull request to the merge queue Dec 16, 2024
Merged via the queue into quinn-rs:main with commit 51e974e Dec 16, 2024
18 checks passed
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.

3 participants