Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add GetEpoch local state query #320

Closed
wants to merge 13 commits into from
Closed

add GetEpoch local state query #320

wants to merge 13 commits into from

Conversation

falcucci
Copy link
Contributor

@falcucci falcucci commented Oct 30, 2023 •
edited
Loading

fixes #315

@falcucci falcucci changed the title implement GetEpoch local state query w examples add GetEpoch local state query Oct 30, 2023
@falcucci falcucci changed the title add GetEpoch local state query addGetEpoch local state query Oct 30, 2023
@falcucci falcucci changed the title addGetEpoch local state query add GetEpoch local state query Oct 30, 2023
@falcucci falcucci marked this pull request as ready for review October 30, 2023 16:58
scarmuega
scarmuega reviewed Oct 31, 2023
View reviewed changes
pallas-network/tests/protocols.rs Outdated
@@ -399,6 +401,150 @@ pub async fn local_state_query_server_and_client_happy_path() {
_ = tokio::join!(client, server);
}

#[tokio::test]
pub async fn local_state_query_server_and_block_query_get_epoch_client_happy_path() {
Copy link
Member

Choose a reason for hiding this comment

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

there's no need to repeat the whole code of the test for each type of query. Put everything in a single happy path test that includes all of the available queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scarmuega done.

@scarmuega
Copy link
Member

@falcucci we're missing part of the requirements of the issue: "we have primitives to represent the response". What I mean by that is that we need to get rid of the GenericResponse and start using structs that represent the decoded version of the query results.

@falcucci falcucci marked this pull request as draft October 31, 2023 10:47
falcucci added 10 commits October 31, 2023 16:07
@falcucci
unify local state query fixtures
b7e434a
@falcucci
minor changes
86aa076 
```diff
diff --git a/pallas-network/tests/protocols.rs b/pallas-network/tests/protocols.rs
index 5812f88838ed..b625632f0f15 100644
--- a/pallas-network/tests/protocols.rs
+++ b/pallas-network/tests/protocols.rs
@@ -290,7 +290,6 @@ pub async fn local_state_query_server_and_client_happy_path() {
                 .unwrap();

             // server receives range from client, sends blocks
-
             let clientacquirerequest(maybe_point) =
                 server_sq.recv_while_idle().await.unwrap().unwrap();

@@ -322,7 +321,6 @@ pub async fn local_state_query_server_and_client_happy_path() {
                 .unwrap();

             // server receives release from the client
-
             match server_sq.recv_while_acquired().await.unwrap() {
                 clientqueryrequest::release => (),
                 x => panic!("unexpected message from client: {x:?}"),
@@ -360,7 +358,6 @@ pub async fn local_state_query_server_and_client_happy_path() {
             assert_eq!(*server_sq.state(), localstate::state::acquired);

             // server receives reaquire from the client
-
             let maybe_point = match server_sq.recv_while_acquired().await.unwrap() {
                 clientqueryrequest::reacquire(p) => p,
                 x => panic!("unexpected message from client: {x:?}"),
@@ -372,7 +369,6 @@ pub async fn local_state_query_server_and_client_happy_path() {
             server_sq.send_acquired().await.unwrap();

             // server receives release from the client
-
             match server_sq.recv_while_acquired().await.unwrap() {
                 clientqueryrequest::release => (),
                 x => panic!("unexpected message from client: {x:?}"),
```
@falcucci
add response primitives
02edf49 
currently supporting only non params local state queries
@falcucci
minor changes.
f5c986a 
```diff
diff --git a/pallas-network/src/miniprotocols/localstate/queries.rs b/pallas-network/src/miniprotocols/localstate/queries.rs
index dabc73e646d0..71921c1672c7 100644
--- a/pallas-network/src/miniprotocols/localstate/queries.rs
+++ b/pallas-network/src/miniprotocols/localstate/queries.rs
@@ -42,7 +42,7 @@ impl encode<()> for blockquery {
         e.u16(0)?;
         e.array(2)?;
         /*
-            todo: i think this is era or something? first fetch era with
+            todo: think this is era or something? first fetch era with
             [3, [0, [2, [1]]]], then use it here?
         */
         e.u16(5)?;
```
@falcucci
renaming blockqueryresponse variants
9b8316b 
renames the variants of the `blockqueryresponse` enum in the `queries.rs` file of the `localstate` miniprotocol.

the following changes were made:

- `getledgertip` is now `ledgertip`
- `stakepools` is now `currentpparams`
- `getcurrentpparams` is now `proposedpparamsupdates`
- `getproposedpparamsupdates` is now `stakedistribution`
- `getstakedistribution` is now `genesisconfig`
- `getgenesisconfig` is now `debugchaindepstate`
- `getrewardprovenance` is now `rewardprovenance`
- `getstakepools` is now `stakepools`
- `getrewardinfopools` is now `rewardinfopools`
@falcucci
mapping response primitives with local request code signals
f73f8fc 
this has been an attempt to map the server requests into our responses.

tests must be updated.
@falcucci
adding epoch_no struct
b58e6aa 
we added the struct. maybe we should adapt to use it as turbofish
@falcucci
fix some imports
72d3d7a 
- changed the argument of the `do_localstate_query` function from `localstate::queries::request` to `request`
- updated the function call in `main` to use `request` instead of `localstate::queries::request`
- removed unnecessary impls for `epochno`
@falcucci
fix tests and types
a0e0225
@falcucci
remove usuless dependency
1b2a8a1
@falcucci falcucci marked this pull request as ready for review November 6, 2023 23:42
@scarmuega
Copy link
Member

closing in favor of #326

@scarmuega scarmuega closed this Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scarmuega scarmuega scarmuega left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: implement GetEpoch local state query
2 participants
@falcucci @scarmuega