Remove /gov/read and /gov/query endpoints
#2442
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 10 commits
April 8, 2021 11:04
jumaffre
commented
Apr 9, 2021
| @@ -312,238 +192,6 @@ struct TestNewMember | |||
| crypto::Pem cert; | |||
| }; | |||
|
|
|||
| DOCTEST_TEST_CASE("Add new members until there are 7 then reject") | |||
There was a problem hiding this comment.
This made use of query and read but I decided to scrap it as it is already covered by end-to-end tests.
jumaffre
commented
Apr 9, 2021
| for table_name, records in tx.get_public_domain().get_tables().items(): | ||
| if table_name in public_tables: | ||
| public_tables[table_name].update(records) | ||
| # Remove deleted keys |
There was a problem hiding this comment.
Note that the latest state does not include keys deleted keys (value of None in delta), although we do include empty tables.
letmaik
approved these changes
Apr 9, 2021
eddyashton
approved these changes
Apr 9, 2021
Comment on lines
+539
to
+540
| for chunk in self: | ||
| for tx in chunk: |
There was a problem hiding this comment.
Non-critical: This iteration is going to be very slow. Could we rely on the filenames here to scan to the relevant chunk, and parse it in isolation? And even within that, could we skip over other transactions rather than parsing them? Not needed for this PR or current tests, but will likely bite us if we start using this ledger util in more places/on real service ledgers.
There was a problem hiding this comment.
Yep, I should have mentioned it in the comment. I wanted the fetching to still verify the integrity of the ledger and we currently have no way to do that without starting from the very beginning of the ledger. I'll create a separate ticket and add a comment in the docstring.
There was a problem hiding this comment.
@eddyashton See first item in #2453 for follow-up improvements.
Co-authored-by: Eddy Ashton <ashton.eddy@gmail.com>
Co-authored-by: Eddy Ashton <ashton.eddy@gmail.com>
|
remove_gov_read_1@23206 aka 20210412.8 vs main ewma over 20 builds from 22883 to 23191 Click to see table
|
added 2 commits
April 12, 2021 09:52
achamayou
pushed a commit
to achamayou/CCF
that referenced
this pull request
Apr 13, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #2411
This PR removes the existing
/gov/readand/gov/queryendpoints which used the Lua interpreter and were mostly used for testing.Instead,
ccf.ledger.Ledgernow has two new membersget_transaction(seqno)to return theccf.ledger.Transactionrecorded in the ledger at a specificseqnoandget_latest_public_state()to return the latest public state. The former is used to check that a specific table was updated by a proposal (hence tracking the proposal completionseqno) while the latter is used to check the final public state (e.g. to check that JWT refresh works).