-
Notifications
You must be signed in to change notification settings - Fork 206
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
feat!: Support publishing and consuming chain storage stream cells #5942
Conversation
.height as $dataHeight | | ||
|
||
# Flatten each value independently. | ||
.values[] | fromjson | |
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.
I wondered if this change had any impact on this script. Do we have any tests that would have failed if we didn't remember to update this script?
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.
I'm pretty sure we don't have any tests of shell scripts, and I think that's fine. They're not exported from the packages and not part of the API of the package. They're just here to help organize the code.
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.
It's perhaps not exported in the npm sense, but it's a customer-visible API (customer-required, in fact). I would like to see an integration test where
- javascript code writes capData to a file
- this script reads the capData file and writes flat data
- we diff the flat data against expected results
Of course, I'd like to see a lot of things... I'm not sure whether it's cost-effective any time soon. It's a question of how often we're likely to tweak this script.
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.
Perhaps it should be exported in the npm sense? that is: perhaps it should go in .bin
?
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.
The problem with a test like you describe above is that it's mostly navel-gazing—there's no connection between the hard-coded input and the kind of output that is expected of RPC nodes in the real world. But I do still thank that having something is a good idea.
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.
OK, I've added shell-based testing. Any ideas for how to make sure they run with the rest of our tests?
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.
The test
nom script could call the shell test thing, maybe? (I haven't looked at the shell thing)
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.
Added a simple runner at scripts/test/test.sh (and a GitHub workflow calling it).
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.
Reviewed the chain JS. I don't know enough about the Go parts to review so not approving the PR.
If you'd like me to dive in enough to do a full review let's discuss because I'll have to reprioritize some stuff.
@@ -64,10 +64,19 @@ const fakeStatusResult = { | |||
}, | |||
}; | |||
|
|||
export const startFakeServer = (t, fakeValues, marshaller = makeMarshal()) => { | |||
/** @typedef {Partial<import('ava').ExecutionContext<{cleanups: Array<() => void>}>> & {context}} fakeServerTestContext */ |
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.
😍
@@ -38,11 +38,14 @@ harden(sanitizePathSegment); | |||
* @param {(message: StorageMessage) => any} handleStorageMessage a function for sending a storageMessage object to the storage implementation (cf. golang/cosmos/x/vstorage/vstorage.go) | |||
* @param {string} storeName currently limited to "swingset" |
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.
drive-by: type is effectively {'swingset'}
// Instantiate chain storage over a simple in-memory implementation. | ||
// makeFakeChainStorageKit creates a chainStorage root node over an in-memory map | ||
// and exposes both the map and the sequence of received messages. | ||
const makeFakeChainStorageKit = (rootPath, options) => { |
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.
We have more than one kind of fake storage. How about makeTransientChainStorageKit
? Not sure we even need "chain" #5945
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.
Also, WDYT of this in lib-chainStorage
for use by other packages?
// makeFakeChainStorageKit creates a chainStorage root node over an in-memory map | ||
// and exposes both the map and the sequence of received messages. |
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.
better as jsdoc. can include param types as well
.height as $dataHeight | | ||
|
||
# Flatten each value independently. | ||
.values[] | fromjson | |
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.
I'm pretty sure we don't have any tests of shell scripts, and I think that's fine. They're not exported from the packages and not part of the API of the package. They're just here to help organize the code.
// but would add a requirement exclusive to AppendStorageValueAndNotify that its input be JSON. | ||
// On the other hand, we could always extend this format in the future to include an indication | ||
// that values are subject to a different encoding, e.g. `"valueEncoding":"base64"`. | ||
Values []string `json:"values"` |
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.
I think string
also limits cells to JSON (or at least text formats) in a way that []byte
would not. I think we should commit to JSON. Strings might be better since interface{} has a lot more liberty to mess up 64 bit integers among other things.
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.
I decided to stick with stream cell values being strings, aligning with the existing interfaces. So from the outside in we have
- JS ChainStorageNode option "sequence" is
false
(default) vs.true
. - JS ChainStorageNode method
setValue(value: string)
sendsvalue
with method "set" vs. "append". - vstorage.go invokes
keeper.SetStorageAndNotify
vs.keeper.AppendStorageValueAndNotify
, in either case with the received value. - AppendStorageValueAndNotify loads or initializes a stream cell
struct { Height string, Values []string }
, appends the value, and callsSetStorageAndNotify
with the cell's JSON serialization (this representation of a stream cell as JSON is the opinionated decision). - SetStorageAndNotify works down to SetStorage, which performs the actual write of value as a []byte with a static prefix.
// otherwise initialize a blank cell. | ||
currentData := k.GetData(ctx, path) | ||
var cell StreamCell | ||
_ = json.Unmarshal([]byte(currentData), &cell) |
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.
Error handling
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.
Intentionally suppressed here because we populate cell
manually if Unmarshal
fails to do so.
return; | ||
let streamCell = decode(buf); | ||
// Upgrade a naked value to a JSON stream cell if necessary. | ||
if (!streamCell.height || !streamCell.values) { |
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.
Is 0 a valid height?
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.
No, because it's not a string. And "0"
is truthy.
// eslint-disable-next-line no-continue | ||
continue; | ||
} | ||
committer.commit({ value }); |
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.
What’s the envelope around value
for? I only see { value }
getting committed, not { value, done }
, the one likely suspect.
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.
I haven't actually figured that out yet.
if (!last) { | ||
committer = prepareUpdateInOrder(); | ||
} |
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.
Why replace the committer only if final?
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.
Only if not final. And I think replacing it in final position would make it look like there are gaps, although I could be wrong.
export const startFakeServer = (t, fakeValues, marshaller = makeMarshal()) => { | ||
/** @typedef {Partial<import('ava').ExecutionContext<{cleanups: Array<() => void>}>> & {context}} fakeServerTestContext */ | ||
/** | ||
* @param {fakeServerTestContext} t |
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.
Is there a reason for the type to have a little f?
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.
No, I'll capitalize it.
/** @typedef {Partial<import('ava').ExecutionContext<{cleanups: Array<() => void>}>> & {context}} fakeServerTestContext */ | ||
/** | ||
* @param {fakeServerTestContext} t | ||
* @param {Array<{any}>} fakeValues |
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.
What does {any}
mean?
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.
IIUC, it imposes no constraints beyond existence.
lastPort += 1; | ||
const PORT = lastPort; |
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.
Aside: this usually tells me we should listen on 0 and ask the kernel for the port we were assigned.
@gibson042
|
@arirubinstein I think that was from |
@arirubinstein I'd also like to rename |
e5f29c9
to
89daa67
Compare
…-flattened-publication.sh Use existence of "blockHeight" and "values" properties rather than truthiness (which differs between JavaScript and `jq`).
672526e
to
2ce41da
Compare
Fixes #5366
Fixes #5508
Description
Adds support for publishing and opportunistically consuming { height, values } chain storage stream cells in place of naked values, and switches the "published" chain storage subtree to use them.
I'm not sure how casting is intended to behave with batches, and confused about the
committer.isValid()
guard.Security Considerations
There's a minor risk of stored data getting confused for a stream cell, but since everything is currently under our control, that should be fine until we can eliminate naked values (at least in the "published" subtree).
Documentation Considerations
I'm not sure how to document the new structure, which I'm calling a stream cell per https://github.com/Agoric/agoric-sdk/tree/mfig-vstream/golang/cosmos/x/vstream/spec .
Testing Considerations
It looks like there are no current tests for the Go aspects, and how to add some isn't clear.