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

Add address: Address to ReducerContext #299

Merged
merged 24 commits into from
Sep 30, 2023
Merged

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Sep 14, 2023

Description of Changes

Initial support for identifying connections via an Address, passed as the caller address in ReducerContext.

Notable design choices:

  • The same type Address is used for clients and dbs, as opposed to having distinct DbAddress and ClientAddress types.
  • init and update receive the address of the caller of publish, or None if the caller did not supply an address. This is useful for SpacetimeDB-cloud.
  • Scheduled reducers do not receive an address.
  • Clients may specify an Address as a query parameter to the subscribe and call endpoints. If they do not, an Address will be randomly generated for them.
  • The Rust SDK always generates a random Address the first time it connects, and uses that Address for the duration of the process. This will allow modules to detect re-connecting clients in the case of an unintentional disconnect, e.g. caused by an intermittent internet connection.

Breaking changes that aren't particularly design decisions:

  • The IdentityToken message adds a new field address, which holds the client's address. This will allow clients which do not generate their own Address to discover the address generated by the server, and clients which do generate their own Address to verify it.
  • The Event message (actually a part of TransactionUpdate) adds a new field callerAddress, which holds the address of the client which called the reducer. The all-zeros address is used as a sentinel for None (same as for host-module communcation).
  • The database info stored in the Sled DB now holds the publisher's address. This is an unfortunate necessity to passing it through from the publish endpoint to the init/update reducer. This also will probably be a merge conflict with cloud-next.
  • Address is now a product type with a single field, __address_bytes. This is similar to how we treat Identity, and allows the CLI's codegen to reference an appropriate SDK-side Address type rather than a u128 or an array of bytes.
  • The Rust SDK's on_connect callbacks (some SDKs call this onIdentityReceived or similar) takes an additional argument, the client's Address.
  • The Rust SDK's reducer callbacks take an additional argument of type Option<Address>, the address of the caller of the reducer.

Still to do:

API and ABI

  • This is a breaking change to the module ABI
  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

ReducerContext contains an additional field, so low-level macro-generated reducer functions take an additional argument.

Technically this is a SemVer breaking change to the module API, as previous exhaustive destructurings of ReducerContext will no longer compile. For modules which don't want the new functionality, they can trivially add a , .. to their patterns.

The Rust SDK's on_connect callbacks (some SDKs call this onIdentityReceived or similar) takes an additional argument, the client's Address.

This is a non-breaking change to the HTTP API, as it adds an optional query parameter to the subscribe and call endpoints.

This adds a field to two of our client API protobuf messages (IdentityToken; Event). I don't understand protobuf deeply enough to know if this counts as a breaking change.

Initial support for identifying connections via an `Address`,
passed as the caller address in `ReducerContext`.

Notable design choices:

- The same type `Address` is used for clients and dbs,
  as opposed to having distinct `DbAddress` and `ClientAddress` types.
- For automatic reducers (init, scheduled, &c), the passed `Address`
  is the database's address, not the address of the client which called `publish`.
- Clients may specify an `Address` as a query parameter
  to the `subscribe` and `call` endpoints.
  If they do not, an `Address` will be randomly generated for them.

Still to do:

- Send the client's `Address` alongside their `Identity` and token
  upon WebSocket init in `IdentityToken`,
  so the client can save its `Address` for re-connection.
- SDK support.
- C# module support.
- Documentation.
- Refuse new connections if an existing connection
  with the same `(Identity, Address)` pair exists.
- Ensure we can store `Address` in tables,
  so modules can track connected clients,
  and ser/de `Address`, so those tables can be synced to clients.
Copy link
Contributor

@kim kim left a comment

Choose a reason for hiding this comment

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

For automatic reducers (init, scheduled, &c), the passed Address is the database's address, not the address of the client which called publish.

Is this true? Could you point me to the code which does this?

As I said elsewhere, I would assume that if those reducers get passed the identity of the caller, then we should also be able to get at the corresponding address.

crates/client-api/src/routes/database.rs Outdated Show resolved Hide resolved
pub client_address: Option<Address>,
}

// TODO: is this a reasonable way to generate client addresses?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this is fine -- as of now, the address is (implicitly) scoped to the identity, so is very unlikely to collide. Unless this fact is not considered, of course.

cloud-next also defines [ControlStateWriteAccess::create_address], so we could use that when porting this patch over.

Copy link
Contributor

@cloutiertyler cloutiertyler Sep 27, 2023

Choose a reason for hiding this comment

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

Why wouldn't init be the caller? I think in this case those two are the same, but I would expect init would be the caller, and that might be meaningful down the road.

crates/client-api/src/routes/subscribe.rs Outdated Show resolved Hide resolved
crates/core/src/host/wasm_common/module_host_actor.rs Outdated Show resolved Hide resolved
@gefjon
Copy link
Contributor Author

gefjon commented Sep 15, 2023

Is this true? Could you point me to the code which does this?

For scheduled reducers:

let address = info.address;

For init:

let caller_address = self.database_instance_context().address;

As I said elsewhere, I would assume that if those reducers get passed the identity of the caller, then we should also be able to get at the corresponding address.

I think this is the wrong intuition; I would say that automatic reducers get passed the identity associated with the database, which is its owner's identity. The person which called publish arguably indirectly causes init/update to be invoked, but I don't think there's any case that they're the caller_address of scheduled or repeating reducers.

Philosophy aside, is there a case where it would be useful for a module to know the Address that published it? I don't think knowing its own Address is currently useful, but I imagine it will become useful when we implement inter-module communication.

Similar to `Identity`, make `Address` a product type with a double-underscored field name.

This will allow SDKs to distinguish `Address` from `Vec<u8>`,
but does necessitate some ugly `AddressForUrl` hackery
to get ser/de working correctly in different contexts.

This commit does not yet include SDK support for `Address`,
only the server-side machinery necessary for it.
@kim
Copy link
Contributor

kim commented Sep 15, 2023

Philosophy aside, is there a case where it would be useful for a module to know the Address that published it? I don't think knowing its own Address is currently useful, but I imagine it will become useful when we implement inter-module communication.

So, the problem is this: with the current authentication model, nodes in the distributed setup have to have the same Identity -- otherwise, we'd need to provision eligible identities more or less manually. So the idea is that the "control" identity is trusted, because it created the database, and every other node will use that same identity but with a different address. That is: the client address in this context is, in fact, a node identity.

Now: in the bootstrap scenario, the init reducer will need to know the node identity of the bootstrap node, so it can assign instances to it.

Does that make sense?

…ucers

Per discussion with Kim, it's useful for SpacetimeDB-cloud
for the client_address passed to the init reducer
to be that of the caller of /database/publish,
rather than the database's address.

This commit implements that behavior. Now:
- `init` and `update` take the client_address passed to publish, if any,
  or `None` if no client_address was supplied.
- Scheduled reducers never receive a client_address.
- Reducers invoked by HTTP /call take the client_address passed to /call, if any,
  or `None` if no client_address was supplied.
- Reducers invoked by WebSocket always receive the address of the WebSocket connection.
@gefjon gefjon force-pushed the phoebe/client-address branch from ec4a91e to cca5c63 Compare September 22, 2023 17:04
gefjon added a commit to clockworklabs/SpacetimeDBLibCSharp that referenced this pull request Sep 26, 2023
Re: clockworklabs/SpacetimeDB#299,
this commit adds the `Address` field to `DbEventArgs`,
and arranges to deserialize it from reducer arguments.
gefjon added a commit to clockworklabs/spacetimedb-python-sdk that referenced this pull request Sep 26, 2023
- Emit `SpacetimeDB.Address` for address types
- Add `callerAddress` field to `ReducerEvent`
gefjon added a commit to clockworklabs/spacetimedb-typescript-sdk that referenced this pull request Sep 27, 2023
@gefjon gefjon marked this pull request as ready for review September 27, 2023 19:32
@gefjon gefjon mentioned this pull request Sep 27, 2023
4 tasks
@cloutiertyler
Copy link
Contributor

I think this is the wrong intuition; I would say that automatic reducers get passed the identity associated with the database, which is its owner's identity. The person which called publish arguably indirectly causes init/update to be invoked, but I don't think there's any case that they're the caller_address of scheduled or repeating reducers.

I'm not so sure about this. I think init/update is really triggered by the caller, not by the database itself, but I could be convinced otherwise. Right now these two are always the same, so it doesn't matter.

@gefjon
Copy link
Contributor Author

gefjon commented Sep 27, 2023

I think init/update is really triggered by the caller, not by the database itsel

Kim convinced me of this offline. The behavior in this PR for automatic reducers is:

  • init gets the client_address passed to the publish HTTP endpoint, or None if no client_address was supplied when publishing.
  • Scheduled reducers always get None as the caller address.

Now that we call connect and disconnect for calls via the HTTP API,
`/database/call` must always have a client_address.
Generate one if none exists.
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

Heroic job with this PR, @gefjon. And the PR comment is 👨‍🍳👌

@cloutiertyler cloutiertyler merged commit 59159db into master Sep 30, 2023
5 checks passed
@RReverser RReverser deleted the phoebe/client-address branch October 2, 2023 12:34
jdetter pushed a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Oct 12, 2023
jdetter pushed a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Oct 12, 2023
jdetter pushed a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Oct 12, 2023
…start client (#54)

* README.md and License.txt updates (#53)

* Update README.md

* Add license.txt

* Rename

* Add LICENSE.txt with proper case

---------

Co-authored-by: Derek Brinkmann <dbrinkmann@clockworklabs.io>

* Add support for client address; re-run `spacetime generate` for quickstart client

Re: clockworklabs/SpacetimeDB#299

---------

Co-authored-by: dbrinkmanncw <109690865+dbrinkmanncw@users.noreply.github.com>
Co-authored-by: Derek Brinkmann <dbrinkmann@clockworklabs.io>
drogus pushed a commit to clockworklabs/spacetimedb-typescript-sdk that referenced this pull request Oct 12, 2023
jdetter added a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Oct 12, 2023
* One-off query support (#49)

* SpacetimeDBURI argument in place of host and ssl enabled bool (#52)

* SpacetimeDBURI argument in place of host and ssl enabled bool

* Small cleanup

---------

Co-authored-by: John Detter <no-reply@boppygames.gg>

* Add support for client address; re-run `spacetime generate` for quickstart client (#54)

* README.md and License.txt updates (#53)

* Update README.md

* Add license.txt

* Rename

* Add LICENSE.txt with proper case

---------

Co-authored-by: Derek Brinkmann <dbrinkmann@clockworklabs.io>

* Add support for client address; re-run `spacetime generate` for quickstart client

Re: clockworklabs/SpacetimeDB#299

---------

Co-authored-by: dbrinkmanncw <109690865+dbrinkmanncw@users.noreply.github.com>
Co-authored-by: Derek Brinkmann <dbrinkmann@clockworklabs.io>

* Making Quickstart Work Again (#57)

* Packaged nuget package, quickstart is now working again

* Reset DBNAME

---------

Co-authored-by: John Detter <no-reply@boppygames.gg>

* OnUnhandledReducerError (#31)

Co-authored-by: Steve <steve@codefics.com>
Co-authored-by: Boppy <no-reply@boppygames.gg>

---------

Co-authored-by: james gilles <jameshgilles@gmail.com>
Co-authored-by: dbrinkmanncw <109690865+dbrinkmanncw@users.noreply.github.com>
Co-authored-by: John Detter <no-reply@boppygames.gg>
Co-authored-by: Phoebe Goldman <phoebe@goldman-tribe.org>
Co-authored-by: Derek Brinkmann <dbrinkmann@clockworklabs.io>
Co-authored-by: SteveBoytsun <100594800+SteveBoytsun@users.noreply.github.com>
Co-authored-by: Steve <steve@codefics.com>
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.

4 participants