-
Notifications
You must be signed in to change notification settings - Fork 109
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
Jeremie/cs module #1719
base: master
Are you sure you want to change the base?
Jeremie/cs module #1719
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
namespace SpacetimeDB; | ||
|
||
public abstract class DbContext<DbView> | ||
where DbView : class, new() | ||
{ | ||
public readonly DbView Db; | ||
|
||
public DbContext() => Db = new(); | ||
|
||
public DbContext(DbView db) => Db = db; | ||
} |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,24 +5,6 @@ namespace SpacetimeDB; | |||||
using SpacetimeDB.Internal; | ||||||
using static System.Text.Encoding; | ||||||
|
||||||
public class ReducerContext | ||||||
{ | ||||||
public readonly Identity Sender; | ||||||
public readonly DateTimeOffset Time; | ||||||
public readonly Address? Address; | ||||||
|
||||||
internal ReducerContext( | ||||||
Identity senderIdentity, | ||||||
Address? senderAddress, | ||||||
DateTimeOffset timestamp | ||||||
) | ||||||
{ | ||||||
Sender = senderIdentity; | ||||||
Address = senderAddress; | ||||||
Time = timestamp; | ||||||
} | ||||||
} | ||||||
|
||||||
// [SpacetimeDB.Type] - we have custom representation of time in microseconds, so implementing BSATN manually | ||||||
public abstract partial record ScheduleAt | ||||||
: SpacetimeDB.TaggedEnum<(DateTimeOffset Time, TimeSpan Interval)> | ||||||
|
@@ -77,6 +59,14 @@ public AlgebraicType GetAlgebraicType(ITypeRegistrar registrar) => | |||||
} | ||||||
} | ||||||
|
||||||
public abstract class BaseReducerContext<DbView> : DbContext<DbView>, IReducerContext | ||||||
where DbView : class, new() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless we have an interface to uphold, I'd rather not spell out extra constraints. In this case it doesn't seem to add much value in terms of checks.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made it a class because it does carry state; the connection handle in this case. A struct could be copied/defaulted and introduce a silent null connection. This forces it to be a reference type, and given there's only one per application the distinction between value/ref is no longer relevant, I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well in this case Local is stateless, but the same base class has to support DbConnection on the client side. |
||||||
{ | ||||||
public Identity Sender => Runtime.SenderIdentity!; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these set on the singleton now? What happens if someone wants to store a I think those should be own fields, like before. For Even for RNG that's changing in #1681 and they're all going to live on the context now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reducer context is created once for the duration of the module, and @gefjon says each reducer invocation should be seen as its own isolated instance with no memory able to outlive that invocation. Making them fields would need a new ReducerContext per reducer invocation, which means its DbView instance has to either also be created per invocation, or stored somewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with either approaches; I guess it depends on what semantics we want for memory across reducer invocations, and if we want to leverage isolation in the impl like this ReducerContext does here. |
||||||
public Address Address => Runtime.SenderAddress!; | ||||||
public DateTimeOffset Timestamp => Runtime.Timestamp!; | ||||||
} | ||||||
|
||||||
public static class Runtime | ||||||
{ | ||||||
public enum LogLevel : byte | ||||||
|
@@ -115,4 +105,10 @@ public static void Log( | |||||
|
||||||
// An instance of `System.Random` that is reseeded by each reducer's timestamp. | ||||||
public static Random Random { get; internal set; } = new(); | ||||||
|
||||||
public static Identity? SenderIdentity { get; internal set; } | ||||||
|
||||||
public static Address? SenderAddress { get; internal set; } | ||||||
|
||||||
public static DateTimeOffset Timestamp { get; internal set; } | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reducer kind being a special name was more of an implementation detail, and that will be changing in the new proposal (WIP in #1670) where ReducerKind is stored separately.
AFAIK the SDK proposal also doesn't allow custom
#[spacetimedb::reducer(name = "...")]
, so we shouldn't be doing that in C# either - can you please revert this to a regular attribute parameter, so it would be easier to migrate to non-name-based ReducerKind in the future.