Skip to content

Add a TokenSource crate to the Rust SDKs #1274

Draft
MaxHeimbrock wants to merge 13 commits into
mainfrom
max/token-source
Draft

Add a TokenSource crate to the Rust SDKs #1274
MaxHeimbrock wants to merge 13 commits into
mainfrom
max/token-source

Conversation

@MaxHeimbrock

Copy link
Copy Markdown
Contributor

Before you submit your PR

Make sure the following is true before submitting your PR:

  • [ x ] I have read the contributing guidelines and validated that this PR will be accepted.
  • [ x ] I have read and followed the principles regarding breaking changes, testing, and code quality.

PR description

This PR adds the Token Source concept as an independent crate.

Testing

Ideally, unit test the code you add, but ensure you're not repeating existing test cases. Use as many already written scaffolding, utilities as possible; write your own, when needed. If external services, APIs, tokens are required (e.g., running an LK server instance), provide the necessary information. Make sure your tests perform useful, context-aware assertions and do not simply emulate "happy paths".

Async

We want the project to be runtime-agnostic, so please reuse what's already in livekit-runtime and feel free to add anything missing. It's ok to use Tokio directly, when writing unit tests, if necessary. When testing, do not use artificial delays for the state to "catch up"; instead, respect the event flow and subscribe properly using channels or other mechanisms.

@github-actions

Copy link
Copy Markdown
Contributor

No changeset found

This PR modifies versioned packages but doesn't include a changeset. The following packages require a version bump:

  • livekit
  • livekit-api
  • livekit-ffi
  • livekit-uniffi

A package must be bumped when its own files change, and whenever a package it depends on is bumped (so downstream consumers get a matching release).

Click here to create a changeset for the missing packages

The link pre-populates a changeset file with patch bumps for the missing packages. You can also add them to your existing changeset. Edit the bump types as needed before committing.

If this change doesn't require a version bump, add the internal label to this PR.

@ladvoc ladvoc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some initial comments, will return for a more thorough review when this is taken out of draft.

Comment thread livekit-token-source/src/request.rs
readme = "README.md"

[dependencies]
reqwest = { workspace = true, features = ["rustls-tls-native-roots"] }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue: Before merging, this should be updated to align with #1258.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You mean the choice of http client? I had a discussion with Claude about this and settled on using reqwest just to unblock myself, but that is one of the todos I wanted to ask others about.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

from sync: See the new livekit-net crate @jhugman just merged.

Comment thread livekit-token-source/src/token_source.rs Outdated

@1egoman 1egoman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I saw you opened this so I left some initial thoughts, feel free to ignore them for now if it's still early and not yet ready for a review.

Comment thread livekit-token-source/src/request.rs
Comment thread livekit-token-source/src/token_source.rs Outdated
Comment thread livekit-token-source/src/token_source.rs Outdated
Comment thread livekit-token-source/src/token_source.rs Outdated
Comment thread livekit-token-source/src/token_source.rs Outdated
.with_agent_name("Church");

// =======================================================
let sandbox = TokenSourceSandbox::new("test1-xqsb8v".to_string());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since we sunset the sandbox concept and name, this should probably named ProjectTokenSource or something else.

We write: "API fields such as sandboxId and token server URLs still use sandbox naming for compatibility."

We can deprecate existing SDK mentions of SandboxTokenSource and forward to the new name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's discuss this maybe with #devex

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or rather talk to the #ocd

@MaxHeimbrock

Copy link
Copy Markdown
Contributor Author

Potential follow up: Mint tokens based on API secret

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