Refactored sequence_number and fixed bidirectional object transfer #50

patrickkuo · 2021-12-11T16:49:50Z

removed sequence_number from ClientState
Added sequence number to object_id, each object have its own sequence id now
update client's object_id and sequence_number in receive_from_fastpay
updated test for bidirectional object transaction

sblackshear · 2021-12-13T00:59:27Z

fastpay/src/config.rs

@@ -98,8 +99,7 @@ pub struct UserAccount {
    )]
    pub address: FastPayAddress,
    pub key: KeyPair,
-    pub next_sequence_number: SequenceNumber,
-    pub object_ids: Vec<ObjectID>,
+    pub object_ids: HashMap<ObjectID, SequenceNumber>,


I use TreeMap instead of HashMap if I'm planning on iterating over the map (as line 114). HashMap iteration order is nondeterministic, which can cause flakiness and other unexpected problems...

sblackshear · 2021-12-13T01:00:42Z

fastpay_core/src/client.rs

-    object_ids: Vec<ObjectID>,
+    received_certificates: BTreeMap<ObjectRef, CertifiedOrder>,
+    /// The known objects with it's sequence number owned by the client.
+    object_ids: HashMap<ObjectID, SequenceNumber>,


Similar here, since line 484 iterates over object_id's.

gdanezis

Nice one. See my comments about the structures to store certs etc. I do wonder if some of the client could reuse structures from the authority logic to (1) make sure the info is stored in the most handy way, and (2) potentially facilitate a unified sync client <-> authority, and authority <-> authority.

gdanezis · 2021-12-20T14:41:38Z

fastpay_core/src/client.rs

+    fn get_spendable_amount(
+        &mut self,
+        object_id: ObjectID,
+    ) -> AsyncResult<'_, Amount, anyhow::Error>;


Am I correct in thinking this function will probably have to go? Since we do not deal with amounts any more.

gdanezis · 2021-12-20T14:43:52Z

fastpay_core/src/client.rs

+                object_id,
+            );
+
+            let mut number = SequenceNumber::from(0);


Do we always start looking at 0, or should we start at the highest known cert here?

this is remnant of fastpay, shouldn't need to loop through all the certs after we change how we store certs in client, will add a TODO

gdanezis · 2021-12-20T14:45:35Z

fastpay_core/src/client.rs

        // Sanity check
+        // Some certificates of the object will be from received_certs if the object is originated from other sender.
+        // TODO: Maybe we should store certificates in one place sorted by object_ref instead of sent/received?


Yes, i think that is a good idea. In fact the structures a client needs to store certs may be remarkable similar to the structures within an authority? Ie, an index from object ref to cert that created it?

gdanezis · 2021-12-20T14:46:22Z

fastpay_core/src/client.rs

@@ -708,4 +706,29 @@ where
            Ok(new_certificate)
        })
    }
+
+    fn get_spendable_amount(


Maybe we should rename this now?

will remove this in Client API expansion task

gdanezis · 2021-12-20T14:46:53Z

fastpay_core/src/client.rs

+                Amount::zero()
+            } else {
+                Amount::try_from(self.balance).unwrap_or_else(|_| std::u64::MAX.into())
+            };*/


... and delete the dead code, which will never be revived.

…m to confirm the client have ownership of an object in test

* Added sequence number to object_id, each object have its own sequence id now * update client's object_id and sequence_number in receive_from_fastpay * updated test for bidirectional object transaction

removed dead code

patrickkuo changed the base branch from main to pat/client_test_refactoring December 11, 2021 16:50

patrickkuo linked an issue Dec 12, 2021 that may be closed by this pull request

[fastx distsys] Refactor Sequence number #37

Closed

sblackshear reviewed Dec 13, 2021

View reviewed changes

patrickkuo force-pushed the pat/client_sequence_number_refacotring branch from f628dcf to 7936233 Compare December 13, 2021 14:35

patrickkuo force-pushed the pat/client_test_refactoring branch from b10ec4f to 20e28f7 Compare December 14, 2021 10:09

patrickkuo force-pushed the pat/client_sequence_number_refacotring branch 3 times, most recently from 66335ed to 8ad8ebf Compare December 17, 2021 11:34

patrickkuo changed the base branch from pat/client_test_refactoring to main December 17, 2021 11:34

patrickkuo force-pushed the pat/client_sequence_number_refacotring branch from 8ad8ebf to 25dbe5c Compare December 17, 2021 19:10

patrickkuo requested review from sblackshear, huitseeker, gdanezis, lxfind and oxade December 20, 2021 12:45

patrickkuo self-assigned this Dec 20, 2021

patrickkuo marked this pull request as ready for review December 20, 2021 12:45

patrickkuo force-pushed the pat/client_sequence_number_refacotring branch from fde71ea to 77123e4 Compare December 20, 2021 12:57

gdanezis approved these changes Dec 20, 2021

View reviewed changes

patrickkuo added 9 commits December 21, 2021 01:32

refactored get_strong_majority_balance to object_ownership_have_quoru…

bb4dffd

…m to confirm the client have ownership of an object in test

removed unnecessary changes

29d527c

address PR comments

5443b43

* removed sequence_number from ClientState

dc9f2a6

* Added sequence number to object_id, each object have its own sequence id now * update client's object_id and sequence_number in receive_from_fastpay * updated test for bidirectional object transaction

address PR comments

ddd984e

fixup after rebase

c14030f

fixup after rebase

aff3901

fixup after rebase

700d630

adding more comments

526e8a6

patrickkuo force-pushed the pat/client_sequence_number_refacotring branch from 77123e4 to 526e8a6 Compare December 21, 2021 01:32

adding more comments

69f1574

removed dead code

patrickkuo merged commit 8acfa8e into main Dec 21, 2021

patrickkuo mentioned this pull request Dec 28, 2021

[fastx distsys] Adapt fastpay client into fastnft client handling objects #7

Closed

patrickkuo deleted the pat/client_sequence_number_refacotring branch January 12, 2022 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored sequence_number and fixed bidirectional object transfer #50

Refactored sequence_number and fixed bidirectional object transfer #50

patrickkuo commented Dec 11, 2021

sblackshear Dec 13, 2021 •

edited

Loading

sblackshear Dec 13, 2021

gdanezis left a comment

gdanezis Dec 20, 2021

gdanezis Dec 20, 2021

patrickkuo Dec 21, 2021

gdanezis Dec 20, 2021

gdanezis Dec 20, 2021

patrickkuo Dec 21, 2021

gdanezis Dec 20, 2021

Refactored sequence_number and fixed bidirectional object transfer #50

Refactored sequence_number and fixed bidirectional object transfer #50

Conversation

patrickkuo commented Dec 11, 2021

sblackshear Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdanezis 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

sblackshear Dec 13, 2021 •

edited

Loading