Skip to content

Historical JS endpoints#2285

Merged
achamayou merged 18 commits into
microsoft:mainfrom
letmaik:letmaik/js-historical-endpoints
Mar 15, 2021
Merged

Historical JS endpoints#2285
achamayou merged 18 commits into
microsoft:mainfrom
letmaik:letmaik/js-historical-endpoints

Conversation

@letmaik
Copy link
Copy Markdown
Member

@letmaik letmaik commented Mar 9, 2021

No description provided.

Comment thread src/apps/js_generic/js_generic.cpp Outdated
Comment on lines +821 to +824
auto kv_tx_id = tx.get_read_tx_id();
ccf::TxID tx_id;
tx_id.view = static_cast<ccf::View>(kv_tx_id.term);
tx_id.seqno = static_cast<ccf::SeqNo>(kv_tx_id.version);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is very annoying. I'm doing this so that I can to_str(). The casts are needed as kv::TxId uses signed, while ccf::TxID uses unsigned ints.

Comment thread samples/apps/forum/src/types/ccf.ts Outdated
@ghost
Copy link
Copy Markdown

ghost commented Mar 9, 2021

letmaik/js-historical-endpoints@20552 aka 20210315.17 vs main ewma over 20 builds from 20148 to 20500
images

Comment thread samples/apps/forum/src/types/ccf.ts Outdated
@letmaik letmaik marked this pull request as ready for review March 10, 2021 18:13
@letmaik letmaik requested a review from a team as a code owner March 10, 2021 18:13
Comment thread samples/apps/logging/js/app.json Outdated
"execute_outside_consensus": "never",
"authn_policies": ["jwt", "user_cert"],
"historical": true,
"readonly": true,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Historical implies readonly, it would be elegant if the config reflected that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

"mode": "readonly|writable|historical" ? Or were you thinking of something different?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I think that'd be good. Paging @eddyashton for API input, but I think this would have the merit of ruling out historical: true, readonly: false and the validation and error that'd need to go with it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I think this is a tri-state enum. I like readonly and historical but not convinced by writable. I think readwrite is what we've used elsewhere (eg tx.rw)? I could also go for canwrite or maywrite or just write, but I think readwrite is best.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

readwrite is fine. I'll do the change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@achamayou @eddyashton Done, please have a final look if the change looks good.

Comment thread src/apps/js_generic/js_generic.cpp Outdated
Comment thread src/apps/js_generic/js_generic.cpp Outdated
@achamayou
Copy link
Copy Markdown
Member

Looks good! Can the logging JS section in the doc and the changelog be expanded accordingly please?

letmaik and others added 2 commits March 11, 2021 09:24
Co-authored-by: Amaury Chamayou <amaury@xargs.fr>
@letmaik
Copy link
Copy Markdown
Member Author

letmaik commented Mar 11, 2021

Looks good! Can the logging JS section in the doc and the changelog be expanded accordingly please?

There's only a C++ section for logging currently. Let's do docs in another PR.

Comment thread CHANGELOG.md
Comment thread cmake/quickjs.cmake
quickjs.enclave
PUBLIC -nostdinc -DCONFIG_VERSION="${QUICKJS_VERSION}" -DEMSCRIPTEN
-DCONFIG_STACK_CHECK
PRIVATE $<$<CONFIG:Debug>:-DDUMP_LEAKS>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just to check, this is saying "when building this target in Debug, set DUMP_LEAKS", right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah

export type Proof = ProofElement[];

export interface Receipt {
signature: string;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we add a jsdoc comments like /** base64-encoded signature of the root by the node identified by nodeId */, and if we did, would it get picked up by sphinx-js? Just wondering, not requesting :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes and yes, see the CryptoKeyPair interface in #2313

Comment thread samples/apps/forum/src/types/ccf.ts
@achamayou achamayou merged commit 2efb4d5 into microsoft:main Mar 15, 2021
@letmaik letmaik deleted the letmaik/js-historical-endpoints branch March 15, 2021 16:05
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