Remove address_to_object_id_hack part 2 #29

patrickkuo · 2021-12-06T15:14:48Z

This PR further refactor the code to remove usage of address_to_object_id_hack, also

removed balance from ClientState
added Vec<ObjectID> to ClientState
replaced get_object_id() with ObjectID::random()

Todos (in future PRs):

remove/ modify unit test to test object transfer instead of balance
remove all reference to balance
~~Unit test for encode and decoding of ObjectID~~ (ObjectID now inherited Move's AccountAddress)

* removed unused amount args * renamed init_state_with_account to init_state_with_object

This reverts commit 2034fec6b8d69908510ce7e2ed184accfb21910e.

* added objects to client state * added object arg field to init_state_with_account function * removed unused amount args * renamed init_state_with_account to init_state_with_object

* removed unused amount args * renamed init_state_with_account to init_state_with_object

This reverts commit 2034fec6b8d69908510ce7e2ed184accfb21910e.

lxfind · 2021-12-08T17:10:31Z

fastpay/src/bench.rs

@@ -124,22 +124,22 @@ impl ClientServerBenchmark {
        let mut account_keys = Vec::new();


With this change, account_keys probably should be renamed to something like acount_objects as it contains both account and object id.

lxfind · 2021-12-08T17:12:12Z

fastpay_core/src/client.rs

@@ -57,7 +54,7 @@ pub struct ClientState<AuthorityClient> {
    received_certificates: BTreeMap<(FastPayAddress, SequenceNumber), CertifiedOrder>,
    /// The known spendable balance (including a possible initial funding, excluding unknown sent


Update comment.

lxfind · 2021-12-08T17:21:24Z

fastpay/src/client.rs

    )
 }

 /// Make one transfer order per account, up to `max_orders` transfers.
 fn make_benchmark_transfer_orders(
    accounts_config: &mut AccountsConfig,
    max_orders: usize,
-) -> (Vec<Order>, Vec<(FastPayAddress, Bytes)>) {
+) -> (Vec<Order>, Vec<(FastPayAddress, ObjectID, Bytes)>) {


I don't think you need both FastPayAddress and ObjectID. FastPayAddress should be no longer needed as we just need ObjectID for sharding. Similarly for other make_benchmark_ methods.

the address is used later on as the recipient's address, we use ObjectID for sharing but we still need Address for the sender and recipient during transfer.

hmm I don't see it though. Can you point me to the code that uses the address from the serialized_orders?

ohhh you are right, address is not needed here, I was confused with the address_keys in bench.rs, thanks!

lxfind · 2021-12-08T17:24:29Z

fastpay/src/config.rs

@@ -213,7 +213,7 @@ impl InitialStateConfig {
                failure::bail!("expecting two columns separated with ':'")
            }
            let address = decode_address(elements[0])?;
-            let balance = elements[1].parse()?;
+            let balance = ObjectID::from_hex_literal(elements[1])?;


Shouldn't be called balance

lxfind · 2021-12-08T17:32:25Z

fastpay_core/src/client.rs

@@ -276,10 +278,9 @@ where
    /// Find the highest balance that is backed by a quorum of authorities.
    /// NOTE: This is only reliable in the synchronous model, with a sufficient timeout value.
    #[cfg(test)]
-    async fn get_strong_majority_balance(&mut self) -> Balance {
+    async fn get_strong_majority_balance(&mut self, object_id: ObjectID) -> Balance {


What's the semantics of this function under the object model? My understanding is that previously it tries to get the lower bound of the balances. But with objects what does it mean?

it doesn't mean much, we have set the balance to 0 in all of our test, it's remnant of fastpay, have created an issue to refactor all of these.

lxfind

LGTM

patrickkuo force-pushed the pat/object_id_hack_refactoring_pt2 branch from 5106867 to 6e404cc Compare December 7, 2021 11:11

patrickkuo changed the base branch from pat/object_id_hack_refactoring to main December 7, 2021 11:12

patrickkuo added 13 commits December 8, 2021 10:07

* added object arg field to init_state_with_account function

a6092c0

* removed unused amount args * renamed init_state_with_account to init_state_with_object

fix format

eaee98c

remove object_id hack from fund_account

2ac2265

Revert "remove object_id hack from fund_account"

892f63d

This reverts commit 2034fec6b8d69908510ce7e2ed184accfb21910e.

fixup after rebase

b1c96e3

* pass object ids into make client instead of calling get_object_id

5195b8d

* added objects to client state * added object arg field to init_state_with_account function * removed unused amount args * renamed init_state_with_account to init_state_with_object

* added object arg field to init_state_with_account function

c661f24

* removed unused amount args * renamed init_state_with_account to init_state_with_object

Revert "remove object_id hack from fund_account"

1f296fe

This reverts commit 2034fec6b8d69908510ce7e2ed184accfb21910e.

remove object_id hack from fund_account

5de9538

added objects to client state

afd7a5e

fixup after rebase

57baf3a

added back accidental removed ignore annotation

f878477

fixup after rebase

6977063

patrickkuo force-pushed the pat/object_id_hack_refactoring_pt2 branch from 6e404cc to 6977063 Compare December 8, 2021 10:35

patrickkuo added 3 commits December 8, 2021 10:39

fixup after rebase

909b87a

use ObjectID::random() instead of get_object_id

5429aad

Fix clippy error

bbf05ef

patrickkuo mentioned this pull request Dec 8, 2021

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

Closed

patrickkuo added 2 commits December 8, 2021 14:12

added comment on loop in download_sent_certificate

a1a6d56

removed object_id_hack in bench, using ObjectID::random() instead

80c1b45

patrickkuo marked this pull request as ready for review December 8, 2021 14:25

patrickkuo requested review from gdanezis, sblackshear, huitseeker and lxfind December 8, 2021 14:25

patrickkuo self-assigned this Dec 8, 2021

lxfind reviewed Dec 8, 2021

View reviewed changes

address PR issues

62d10b6

address PR issues

af41cad

lxfind approved these changes Dec 8, 2021

View reviewed changes

patrickkuo merged commit 65247d7 into main Dec 8, 2021

patrickkuo deleted the pat/object_id_hack_refactoring_pt2 branch December 8, 2021 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove address_to_object_id_hack part 2 #29

Remove address_to_object_id_hack part 2 #29

patrickkuo commented Dec 6, 2021 •

edited

Loading

lxfind Dec 8, 2021

lxfind Dec 8, 2021

lxfind Dec 8, 2021

patrickkuo Dec 8, 2021

lxfind Dec 8, 2021

patrickkuo Dec 8, 2021

lxfind Dec 8, 2021

lxfind Dec 8, 2021

patrickkuo Dec 8, 2021 •

edited

Loading

lxfind left a comment

		@@ -124,22 +124,22 @@ impl ClientServerBenchmark {
		let mut account_keys = Vec::new();

		@@ -57,7 +54,7 @@ pub struct ClientState<AuthorityClient> {
		received_certificates: BTreeMap<(FastPayAddress, SequenceNumber), CertifiedOrder>,
		/// The known spendable balance (including a possible initial funding, excluding unknown sent

Remove address_to_object_id_hack part 2 #29

Remove address_to_object_id_hack part 2 #29

Conversation

patrickkuo commented Dec 6, 2021 • edited Loading

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

patrickkuo Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

lxfind left a comment

Choose a reason for hiding this comment

patrickkuo commented Dec 6, 2021 •

edited

Loading

patrickkuo Dec 8, 2021 •

edited

Loading