Cosmos `x/vstream` implementation #5466

michaelfig · 2022-05-30T02:05:54Z

refs: #5487

Description

On-chain stream publishing for the client described in @agoric/streams. See the x/vstream spec

Security Considerations

Documentation Considerations

Testing Considerations

JimLarson

Preview of comments. More to come, hopefully this weekend.

JimLarson · 2022-06-06T21:39:47Z

golang/cosmos/proto/agoric/types/stream.proto

+    ];
+
+    // The subkey under which the cell is stored.
+    bytes store_subkey = 3 [


This is the subkey/child key for vstorage? Make that explicit, and be consistent with the choice of "sub" vs "child" made for vstorage.

No, this is at a lower level than vstorage: the Cosmos SDK multistore. I've noted as such.

Hmm, "subkey" doesn't appear to be standard cosmos terminology - the APIs just use "key".

golang/cosmos/proto/agoric/types/stream.proto

JimLarson · 2022-06-07T21:31:34Z

golang/cosmos/proto/agoric/types/stream.proto

+        (gogoproto.moretags)   = "yaml:\"end_state\""
+    ];
+
+    // The prior position in the stream.


Add If this is the first cell in the stream, this will be the default value. All other cells will have a reference with a nonzero block height.

JimLarson · 2022-06-08T18:58:54Z

golang/cosmos/proto/agoric/types/stream.proto

+    ];
+
+    // The sequence number of the value at this position.
+    uint64 sequence_number = 4 [


I first thought this was redundant with firstSequenceNumber in StreamCell, but now I see this is necessary for a reference to a fork point within a cell.

I see you've removed firstSequenceNumber from StreamCell. I guess it's redundant as long as you're always accessing a cell via a StreamPosition, but it seems a little too bold to leave this essential information only extrinsic. Consider restoring the field.

JimLarson · 2022-06-08T19:00:00Z

golang/cosmos/types/stream/cell.go

+	agoric "github.com/Agoric/agoric-sdk/golang/cosmos/types"
+)
+
+func NewStreamCell(blockHeight int64, seq uint64) StreamCell {


Document that seq is the first sequence number to use in the cell.

I changed this to take a prior position. It's better not to leak the sequence number abstraction.

JimLarson · 2022-06-09T16:55:36Z

golang/cosmos/proto/agoric/types/stream.proto

+    ];
+}
+
+// StreamPosition is data that refers to a particular stream cell value.


Add The default value of this message is the equivalent of a null pointer.

JimLarson · 2022-06-09T16:59:47Z

golang/cosmos/types/stream/cell.go

+		priorCell.UpdatedBlockHeight,
+		state.StoreName(),
+		state.StoreSubkey(),
+		uint64(len(priorCell.Values)),


Don't you need to add priorCell.FirstSequenceNumber? Add a test which would have caught this.

Turns out that was redundant information that I've removed.

Not fixed yet.

golang/cosmos/types/stream/cell.go

JimLarson · 2022-06-09T21:52:44Z

golang/cosmos/types/stream/cell.go

+
+func GetLatestPosition(ctx sdk.Context, state agoric.StateRef) (*StreamPosition, error) {
+	var priorCell StreamCell
+	if state.Exists(ctx) {


Consider testing for !state.Exists(ctx) and returning NewZeroStreamPosition(). It looks like a non-existing cell results in the return of a zero stream position, but I think it's safer to make this explicit rather than relying on undocumented, implicit behavior.

JimLarson · 2022-06-09T21:54:34Z

golang/cosmos/types/state_ref.go

+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+// An interface to allow updating KVStore-backed state and extracting proof


An interface ... to a stored StreamCell.

It's not StreamCell specific.

Okay, but see below for comments that make it explicit when we are expecting it to reference a StreamCell. When do you think the SDK might start using generics?

type StateRef[T Jsoner] interface { Read(ctx sdk.Context) (T, error) Write(ctx sdkContext, value T) error ... }

JimLarson

Here's my final comments on the code. I'll address the spec documents in another PR - probably after I've seen more of how this all will be used.

JimLarson · 2022-06-13T22:37:16Z

golang/cosmos/proto/agoric/types/stream.proto

+    // If state=END_STATE_FAILURE, the last value is the serialised error.
+    repeated bytes values = 1;
+
+    // The block height in which this cell was last updated.


Hmm. In the comment at least, instead of "was last updated", try "was produced", and see if that inspires a different name.

JimLarson · 2022-06-13T23:01:53Z

golang/cosmos/proto/agoric/types/stream.proto

+    ];
+
+    // The subkey under which the cell is stored.
+    bytes store_subkey = 3 [


Hmm, "subkey" doesn't appear to be standard cosmos terminology - the APIs just use "key".

JimLarson · 2022-06-13T23:04:16Z

golang/cosmos/proto/agoric/types/stream.proto

+    ];
+
+    // The sequence number of the value at this position.
+    uint64 sequence_number = 4 [


I see you've removed firstSequenceNumber from StreamCell. I guess it's redundant as long as you're always accessing a cell via a StreamPosition, but it seems a little too bold to leave this essential information only extrinsic. Consider restoring the field.

JimLarson · 2022-06-14T06:16:52Z

golang/cosmos/types/stream/cell.go

+)
+
+// NewStreamCell creates a new StreamCell at blockHeight with the specified
+// prior position.


Use a zero stream position if this is the first cell in the stream.

JimLarson · 2022-06-14T06:22:18Z

golang/cosmos/types/stream/cell.go

+}
+
+// GetLatestPosition returns the position of the last value in the cell, or a
+// zero stream position if the referenced cell does not exist.


Since a StateRef could reference anything, document that it is expected to reference a StreamCell.

Add a note that it will unconditionally return the position of the last value in the cell regardless of the cell's end state. The cell end state should be checked to ensure the position is meaningful, e.g. not END_STATE_FAILURE.

JimLarson · 2022-06-14T22:39:12Z

golang/cosmos/proto/agoric/types/stream.proto

+
+option go_package = "github.com/Agoric/agoric-sdk/golang/cosmos/types/stream";
+
+// StreamCell represents one publish state in a vstream stream.


"published". Consider instead "StreamCell contains the stream values that are published during the execution of a single block. Each StreamCell contains a reference to the prior cell in the stream.

JimLarson · 2022-06-14T22:55:04Z

golang/cosmos/proto/agoric/types/stream.proto

+import "gogoproto/gogo.proto";
+
+option go_package = "github.com/Agoric/agoric-sdk/golang/cosmos/types/stream";
+


Introductory comment: "A stream is a sequence of arbitrary bytes values used by publishers to communicate with consumers. Values in a stream are indexed by 1-based contiguous sequence numbers. A stream is stored as a sequence of StreamCells. StreamCells are typically written to the same key in a KVStore, relying on reading at old block heights to see older values."

JimLarson · 2022-06-14T23:29:08Z

golang/cosmos/types/state_ref.go

+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+// An interface to allow updating KVStore-backed state and extracting proof


Okay, but see below for comments that make it explicit when we are expecting it to reference a StreamCell. When do you think the SDK might start using generics?

type StateRef[T Jsoner] interface { Read(ctx sdk.Context) (T, error) Write(ctx sdkContext, value T) error ... }

JimLarson · 2022-06-15T00:28:26Z

golang/cosmos/types/state_ref.go

+
+var _ StateRef = KVStoreStateRef{}
+
+func NewKVStoreStateRef(storeKey sdk.StoreKey, subkey []byte) *KVStoreStateRef {


Consider not pointer-izing.

JimLarson · 2022-06-15T01:42:45Z

golang/cosmos/types/stream/operation.go

+var _, _ StreamCellUpdater = AppendStreamCellUpdater{}, FailStreamCellUpdater{}
+
+type StreamOperation struct {
+	state   agoric.StateRef


Document that state is expected to reference a StreamCell.

michaelfig · 2023-01-06T18:39:45Z

@gibson042 did a great job of implementing this feature mostly in JS with a light touch in Go, so this PR is superseded. Closing.

michaelfig added the agoric-cosmos label May 30, 2022

michaelfig self-assigned this May 30, 2022

michaelfig mentioned this pull request May 30, 2022

Chain storage revamp #5465

Merged

michaelfig requested a review from JimLarson May 30, 2022 03:22

michaelfig marked this pull request as draft May 30, 2022 03:22

michaelfig force-pushed the mfig-vstream branch 4 times, most recently from 27e7cc4 to 1f47045 Compare May 30, 2022 23:21

michaelfig mentioned this pull request Jun 1, 2022

feat(casting): @agoric/casting MVP #5487

Merged

michaelfig force-pushed the mfig-chain-storage-revamp branch from d82718e to 3ad5774 Compare June 5, 2022 04:33

Base automatically changed from mfig-chain-storage-revamp to master June 5, 2022 05:09

JimLarson suggested changes Jun 10, 2022

View reviewed changes

michaelfig added 10 commits June 10, 2022 14:23

feat(vstream): sketch out the apis

16fb283

docs(vstream): document the vstream spec

594f2a9

fix(vstream): decouple the prior pointer

122c971

feat(vstream): clearer separation of vstream from vstorage

523cfc9

refactor(vstream): move more things to powerless functions

ebf71b9

feat(vstream): separate keeperless portion into types

5a8bc84

fix(vstream): ensure we have byte-slice failures

d2db651

feat(vstorage): also emit new-style state_change events

cacfefd

feat(vstream): add sequence numbers and timeout

2049d4f

docs(vstream): add a use cases section

7018a24

michaelfig force-pushed the mfig-vstream branch from 4b7a8e3 to 7018a24 Compare June 10, 2022 20:24

gibson042 and others added 2 commits June 13, 2022 23:43

docs(vstream): Fix typos in Concepts

6d5e5e9

fix(stream): sequence number calculation

69c3315

JimLarson reviewed Jun 15, 2022

View reviewed changes

michaelfig closed this Jan 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cosmos `x/vstream` implementation #5466

Cosmos `x/vstream` implementation #5466

michaelfig commented May 30, 2022 •

edited

Loading

JimLarson left a comment

JimLarson Jun 6, 2022

michaelfig Jun 10, 2022

JimLarson Jun 13, 2022

JimLarson Jun 7, 2022

JimLarson Jun 8, 2022

JimLarson Jun 13, 2022

JimLarson Jun 8, 2022

michaelfig Jun 10, 2022

JimLarson Jun 9, 2022

JimLarson Jun 9, 2022

michaelfig Jun 10, 2022

JimLarson Jun 14, 2022

JimLarson Jun 9, 2022

JimLarson Jun 9, 2022

michaelfig Jun 10, 2022

JimLarson Jun 14, 2022

JimLarson left a comment

JimLarson Jun 13, 2022

JimLarson Jun 13, 2022

JimLarson Jun 13, 2022

JimLarson Jun 14, 2022

JimLarson Jun 14, 2022

JimLarson Jun 14, 2022

JimLarson Jun 14, 2022

JimLarson Jun 14, 2022

JimLarson Jun 15, 2022

JimLarson Jun 15, 2022

michaelfig commented Jan 6, 2023


		option go_package = "github.com/Agoric/agoric-sdk/golang/cosmos/types/stream";

		// StreamCell represents one publish state in a vstream stream.

		import "gogoproto/gogo.proto";

		option go_package = "github.com/Agoric/agoric-sdk/golang/cosmos/types/stream";


		var _ StateRef = KVStoreStateRef{}

		func NewKVStoreStateRef(storeKey sdk.StoreKey, subkey []byte) *KVStoreStateRef {

Cosmos x/vstream implementation #5466

Cosmos x/vstream implementation #5466

Conversation

michaelfig commented May 30, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

JimLarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimLarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig commented Jan 6, 2023

Cosmos `x/vstream` implementation #5466

Cosmos `x/vstream` implementation #5466

michaelfig commented May 30, 2022 •

edited

Loading