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

Rework Rust SDK for new API #1692

Merged
merged 29 commits into from
Sep 30, 2024
Merged

Rework Rust SDK for new API #1692

merged 29 commits into from
Sep 30, 2024

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Sep 10, 2024

Description of Changes

The Rust SDK rework for the new stable API!

Still TODO, to be follow-ups:

  • Subscription modification / mutable subscription sets. This is waiting on actually implementing the internals.
  • Identity and auth rework. This is waiting on that proposal being accepted and implemented.
  • Non-unique BTree indices.
  • Supporting PKs of arbitrary SATS types. Currently we impose Hash + Eq because it's easier.

Reworks the Rust SDK to match the new stable API. This includes a significant rewrite of the SDK internals, since the previous internals were ill-suited to implement the new API.

API and ABI breaking changes

Oh baby it sure is!

Expected complexity level and risk

3 - A pretty major rewrite of the Rust SDK, but we have reasonable test coverage.

Testing

  • SDK tests rewritten and pass.
    • Except resubscribe, which is currently disabled and would be broken.
  • Ran quickstart-chat and played around a bit.

Enough of the Rust SDK rework to run `quickstart-chat`.

Still TODO:
- Implement new codegen.
  Currently I have manually written the bindings for `quickstart-chat`.
  A future commit will update the CLI codegen to generate files
  similar to those I wrote by hand.
- Subscription modification / mutable subscription sets.
  This is waiting on actually implementing the internals.
- Identity and auth rework.
  This is waiting on that proposal being accepted and implemented.
- Non-unique BTree indices.
  `quickstart-chat` happens to not use any of these.
- Supporting PKs of arbitrary SATS types.
  Currently we impose `Hash + Eq` because it's easier.
- Docs, both internal and external.
- Updating the SDK tests.
@gefjon gefjon added api-break A PR that breaks some user-visible API release-0.12 labels Sep 10, 2024
@gefjon gefjon self-assigned this Sep 10, 2024
At least, enough codegen to run quickstart-chat
So that codegen can emit separate table defns and type defns for them
May restore it at some point, but for now I'm not updating it.
…ust-sdk

This involved a fairly significant re-rewrite of Rust codegen,
as we had two incopatible parent branches,
neither of which was the desired version.
`master` had the input changes for the new `ModuleDef`,
and this branch had the output changes for the new SDK.
Post-merge, this branch has both the input and the output changes.
Currently failing because I haven't (re-)implemented graceful disconnect.
This will be reworked after the identity changes,
but those will not be in the 0.12 release.
Any WS message which invokes multiple callbacks will do so with a lock held,
which means that any callback which tries to get the lock will deadlock.
As written, `ctx.identity()` tries to get the lock.
At least, specifically with Rust modules, since I can't run dotnet...
- Update insta snap for new Rust codegen
- Don't pass a non-default namespace to Rust or TypeScript codegen in tests.
  Rust errors if you do; TypeScript should error but doesn't.
@gefjon gefjon marked this pull request as ready for review September 24, 2024 16:03
@kim kim self-requested a review September 24, 2024 17:43
I'd still like to add doc comments to the codegen
Copy link
Collaborator

@coolreader18 coolreader18 left a comment

Choose a reason for hiding this comment

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

I don't think I have the familiarity with the sdk (nor the time) to review & approve the entire thing, but from a Table API and codegen perspective, LGTM.

@gefjon
Copy link
Contributor Author

gefjon commented Sep 27, 2024

I don't think I have the familiarity with the sdk (nor the time) to review & approve the entire thing, but from a Table API and codegen perspective, LGTM.

Luckily we have pretty good test coverage to demonstrate that the thing works, so my main concern is that it matches the proposal and the other impls.

@gefjon gefjon added this pull request to the merge queue Sep 30, 2024
Merged via the queue into master with commit b7d2e3d Sep 30, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break A PR that breaks some user-visible API release-0.12
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants