[fastx client] Use new client functions, and remove old logic (Part 2) #381

gdanezis · 2022-02-07T20:37:46Z

The final step in closing:

[fastx client] Logic of CertificateRequester and other places to get previous certificates is incorrect. #290
[fastx client] Synchronise certificates with authorities before making move call and publish #252

Future PR:

Replace the deleted sync tests with the new (non-fragile) approach with test helper functions.

huitseeker · 2022-02-08T19:22:09Z

Adding @lxfind to review requests for alignment on direction of authority_aggregator.

huitseeker

OK, so while I love red diffs as much as the next overworked dev, the thing that would help me figure out whether out coverage goes down the drain is this:

for every #[test] fn overlong_test_about_frobnication,
the test is transformed into // TODO: test frobnication in Part 2,
then part 3 deletes the comment in question and replaces it with
#[test] fn sleek_frobnication_test

That way as a reviewer, I can focus on those tests not replaced by a placeholder (deleted) and check that the deletion makes sense.

huitseeker · 2022-02-08T19:23:29Z

fastpay_core/src/unit_tests/client_tests.rs

@@ -766,184 +764,6 @@ async fn test_client_state_sync_with_transferred_object() {
    assert_eq!(1, client2.store().certificates.iter().count());
 }

-#[tokio::test]
-async fn test_client_certificate_state() {


Have we chosen a path on what we want to do with sequence numbers yet?

No but I think we keep them for the moment. This test however is heavily relying on the assumption that the client maintain a list of all certificates of its objects, and upon updating authorities it downloads all certificates. This was used to replay the certificates in a sequence -- which is the fastpay way of updating. I will not fix this test, but rather will go and delete any client logic that still relies on this linear-certificate logic.

This is the essence of the #290 issue this is resolving.

huitseeker · 2022-02-08T19:25:37Z

fastpay_core/src/unit_tests/client_tests.rs

@@ -3065,6 +1715,7 @@ async fn client_object(client: &mut dyn Client, object_id: ObjectID) -> (ObjectR
 }

 // A helper function to make tests less verbose
+#[allow(dead_code)]


Maybe making this pub(crate) would help?

Actually this creates more issues than it solves, due to private types leaking.

huitseeker · 2022-02-08T19:26:12Z

fastpay_core/src/unit_tests/client_tests.rs

-    // Get the object created from the call
-    let (new_obj_ref, _) = order_effects.created[0];
+    let (_, published_effects) = pub_res.unwrap();
+    // let published_effects = order_info_resp.signed_effects.unwrap().effects;


cleanup the comment?

Sorry, cleaned these up now (there were many).

huitseeker · 2022-02-08T19:28:10Z

fastpay_core/src/unit_tests/client_tests.rs

-}
-
-#[tokio::test]
-async fn test_transfer_pending_orders() {


Doesn't that leave the whole pending_orders logic untested (however imperfectly) ?

Oops -- good catch. I took this one out by mistake.

Now its back, but less specific.

lxfind · 2022-02-08T19:37:18Z

So happy to see all the old code getting deleted!

lxfind · 2022-02-08T19:38:19Z

fastpay_core/src/authority_aggregator.rs

@@ -520,7 +452,9 @@ where

    /// This function returns a map between object references owned and authorities that hold the objects
    /// at this version, as well as a list of authorities that responsed to the query for the objects owned.
-    pub async fn get_all_owned_objects(
+    ///
+    /// We do not expose this function to users, as its output is hard for callers to interpet.


Update doc comment on the return value as well?

Not sure what you mean?

I mean the doc comment of this function seems out-of-dated and we should update them to reflect the current semantics of the return values.

lxfind · 2022-02-08T19:39:18Z

fastpay_core/src/authority_aggregator.rs

+        ),
+        FastPayError,
+    > {
+        // First get a map of all objects at least a quorum of authorities think we hold.


comment lost

So sorry, rebase troubles

gdanezis · 2022-02-08T21:36:52Z

OK, so while I love red diffs as much as the next overworked dev, the thing that would help me figure out whether out coverage goes down the drain is this:

for every #[test] fn overlong_test_about_frobnication,

the test is transformed into // TODO: test frobnication in Part 2,

then part 3 deletes the comment in question and replaces it with
#[test] fn sleek_frobnication_test

That way as a reviewer, I can focus on those tests not replaced by a placeholder (deleted) and check that the deletion makes sense.

Made an issue to track re-introducing the meaningful tests removes (I already brought back the one deleted by mistake): #396

lxfind

LGTM!

lxfind · 2022-02-09T04:27:02Z

fastpay_core/src/authority_aggregator.rs

@@ -520,7 +452,9 @@ where

    /// This function returns a map between object references owned and authorities that hold the objects
    /// at this version, as well as a list of authorities that responsed to the query for the objects owned.
-    pub async fn get_all_owned_objects(
+    ///
+    /// We do not expose this function to users, as its output is hard for callers to interpet.


I mean the doc comment of this function seems out-of-dated and we should update them to reflect the current semantics of the return values.

gdanezis marked this pull request as draft February 7, 2022 20:37

gdanezis force-pushed the client_cleanup_aggregator branch from 781c7af to d39fc59 Compare February 8, 2022 08:10

gdanezis changed the title ~~[fastx client] Use new client functions, and remove old logic~~ [fastx client] Use new client functions, and remove old logic (Part 2) Feb 8, 2022

gdanezis force-pushed the sync-gives-certs branch from 0bd6dae to 9b2b5e5 Compare February 8, 2022 12:13

gdanezis marked this pull request as ready for review February 8, 2022 12:13

gdanezis linked an issue Feb 8, 2022 that may be closed by this pull request

[fastx client] Logic of CertificateRequester and other places to get previous certificates is incorrect. #290

Closed

gdanezis mentioned this pull request Feb 8, 2022

[fastx client] Logic of CertificateRequester and other places to get previous certificates is incorrect. #290

Closed

gdanezis force-pushed the client_cleanup_aggregator branch from 60ac64a to 8a5a772 Compare February 8, 2022 17:05

gdanezis mentioned this pull request Feb 8, 2022

[fastx client] Correct generic processing of orders and certificates. (Part 1) #378

Merged

Base automatically changed from client_cleanup_aggregator to main February 8, 2022 17:49

gdanezis force-pushed the sync-gives-certs branch from 9b2b5e5 to 795e7fd Compare February 8, 2022 18:33

huitseeker requested review from lxfind, sblackshear and oxade February 8, 2022 19:21

huitseeker reviewed Feb 8, 2022

View reviewed changes

lxfind reviewed Feb 8, 2022

View reviewed changes

huitseeker mentioned this pull request Feb 8, 2022

chore: start replacement FastNFT => Sui #395

Merged

lxfind approved these changes Feb 9, 2022

View reviewed changes

George Danezis added 9 commits February 9, 2022 10:36

Added testing shorthands + test for get_all_owned_objects

10303cb

Added test for sync_all_owned_objects

d078102

Use local struct

47b9bb5

Make fmt happy

baa954b

Simplify test functions

52f3fed

Separate sync owned objects from sync selected

0aaf085

Added logic for order and certificate

7193b67

Use new lgic for sync with authorities

2340edf

Delete old functions

be8b281

George Danezis added 8 commits February 9, 2022 10:36

Remove requester and downloader

b41c960

Make fmt happy

f5bfaa3

Fmt

dd78ae1

Make fmt happy

65051c4

Add back the pending order test

feb26c2

Added review suggestions

e7d5d22

Added a comment

4cf64a8

Make fmt happy

3bc3287

gdanezis force-pushed the sync-gives-certs branch from 189b40c to 3bc3287 Compare February 9, 2022 10:39

gdanezis merged commit e669843 into main Feb 9, 2022

gdanezis deleted the sync-gives-certs branch February 9, 2022 10:57

mwtian pushed a commit that referenced this pull request Sep 12, 2022

Nits (#381)

0ac0391

mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022

Nits (MystenLabs#381)

50d7567

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fastx client] Use new client functions, and remove old logic (Part 2) #381

[fastx client] Use new client functions, and remove old logic (Part 2) #381

gdanezis commented Feb 7, 2022 •

edited

Loading

huitseeker commented Feb 8, 2022

huitseeker left a comment

huitseeker Feb 8, 2022

gdanezis Feb 8, 2022

gdanezis Feb 8, 2022

huitseeker Feb 8, 2022

gdanezis Feb 8, 2022

gdanezis Feb 8, 2022

huitseeker Feb 8, 2022

gdanezis Feb 8, 2022

huitseeker Feb 8, 2022

gdanezis Feb 8, 2022

gdanezis Feb 8, 2022

lxfind commented Feb 8, 2022

lxfind Feb 8, 2022

gdanezis Feb 8, 2022

lxfind Feb 9, 2022

gdanezis Feb 9, 2022

lxfind Feb 8, 2022

gdanezis Feb 8, 2022

gdanezis commented Feb 8, 2022

lxfind left a comment

lxfind Feb 9, 2022

[fastx client] Use new client functions, and remove old logic (Part 2) #381

[fastx client] Use new client functions, and remove old logic (Part 2) #381

Conversation

gdanezis commented Feb 7, 2022 • edited Loading

huitseeker commented Feb 8, 2022

huitseeker 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

lxfind commented Feb 8, 2022

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

gdanezis commented Feb 8, 2022

lxfind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdanezis commented Feb 7, 2022 •

edited

Loading