Skip to content

refactor: make token a watch channel #39

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

Merged
merged 6 commits into from
Jul 1, 2025
Merged

Conversation

prestwich
Copy link
Member

No description provided.

Copy link
Member Author

prestwich commented Jun 20, 2025

@prestwich prestwich marked this pull request as ready for review June 20, 2025 12:49
@prestwich prestwich force-pushed the prestwich/token-channel branch 2 times, most recently from 11a4528 to 5b84070 Compare June 27, 2025 12:57
@prestwich prestwich changed the base branch from evalir/tx-cache-wrapper to graphite-base/39 June 27, 2025 13:38
@prestwich prestwich force-pushed the prestwich/token-channel branch from 5b84070 to 1037b24 Compare June 27, 2025 13:38
@prestwich prestwich changed the base branch from graphite-base/39 to main June 27, 2025 13:38
Comment on lines +198 to +205
/// blocking, panics, errors, and cancel safety. However, it uses a clone
/// of the [`watch::Receiver`] and will not update the local view of the
/// channel.
Copy link
Member

Choose a reason for hiding this comment

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

The actual plus for this fn is that this:

  • does not produce a potentially blocking borrow for the sender half no? (as the secret is inspected and the ownership is transferred, this is safe to hold for a longer amount of time)
  • does not mark the actual token value as seen for all subsequent copies of the channel until the token is refreshed, as the receiver is cloned

no?

Copy link
Member Author

Choose a reason for hiding this comment

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

does not mark the actual token value as seen for all subsequent copies of the channel until the token is refreshed, as the receiver is cloned

this is not really relevant, as we don't have any situations where we wait for the next updated token. We always care only about the immediate value. If we hypothetically wanted to take some action on token refresh, we would care about updating

Copy link
Member Author

Choose a reason for hiding this comment

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

I see the actual + for this function compared to the previous version with the rwlock that you can .await it before auth has been completed

Copy link
Member Author

Choose a reason for hiding this comment

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

the other thing here is not requiring &mut. documenting that it won't update the channel is more about letting people know a side effect of using &self. We don't want consumers to need &mut

@prestwich
Copy link
Member Author

please don't add merge commits they make it harder to resolve issues when things go wrong

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

lgtm

@prestwich prestwich force-pushed the prestwich/token-channel branch from 412711a to 192b1b5 Compare July 1, 2025 18:21
@prestwich prestwich force-pushed the prestwich/token-channel branch from 192b1b5 to d0077f3 Compare July 1, 2025 18:22
@Evalir Evalir merged commit 6ae1e33 into main Jul 1, 2025
7 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.

2 participants