Basic module publishing and move calls from CLI. #133

oxade · 2022-01-07T01:29:29Z

Adding support for Move calls and module publishing from user CLI

Example publish:

cargo build --release
cd target/release
rm -f *.json *.toml
rm -rf db*
# Make config files
for I in 1 2 3 4
do
    mkdir ./db"$I"
    ./server --server server"$I".json generate --host 127.0.0.1 --port 9"$I"00 --database-path ./db"$I" >> committee.json
done

# Create configuration files for 100 user accounts, with 4 gas objects per account and 200 value each.
# * Private account states are stored in one local wallet `accounts.json`.
# * `initial_accounts.toml` is used to mint the corresponding initially randomly generated (for now) objects at startup on the server side.
./client --committee committee.json --accounts accounts.json create-accounts --num 100 \
--gas-objs-per-account 4 --value-per-per-obj 200 initial_accounts.toml
# Start servers
for I in 1 2 3 4
do
    ./server --server server"$I".json run --initial-accounts initial_accounts.toml --committee committee.json &
done

# Query (locally cached) object info for first and last user account
ACCOUNT1=`./client --committee committee.json --accounts accounts.json query-accounts-addrs | head -n 1`

# Get the first ObjectId for Account1 (currently gas)
ACCOUNT1_OBJECT1=`./client --committee committee.json --accounts accounts.json query-objects "$ACCOUNT1" | head -n 1 |  awk -F: '{ print $1 }'`

# Publish 
 ./client --committee committee.json --accounts accounts.json publish $ACCOUNT1_OBJECT1 --path ../../fastx_programmability/framework/sources

TODO:

Add tests
Retrieve the module object ref after a publish
Define the CLI for convenient calling
Handle move call responses well
Incorporate this properly [hack] workaround to boostrap gas coins in genesis #156

oxade · 2022-01-07T01:31:49Z

fastpay/src/client.rs

+            path,
+            gas_object_id,
+        } => {
+            let sender = decode_address(&account).expect("Failed to decode sender's address");


Need to cleanup gas handling logic here

gdanezis

You are right that the object_id and seq_num of orders make no sense any more.

gdanezis · 2022-01-07T17:06:55Z

fastpay/src/client.rs

+        path: String,
+
+        /// ID of the gas object for gas payment, in 20 bytes Hex string
+        gas_object_id: String,


Do we need to provide both the gas_object and the account? The gas object presumably has an owner, whose account is assumed to be used no?

gdanezis · 2022-01-07T17:08:07Z

fastpay/src/client.rs

-                let objects_ids = client_state.object_ids();
+                // Try to sync.
+                // TODO: figure out optimal tuning for high probability of getting full data back
+                for _ in 0..(1 + committee_config.authorities.len() / 3) {


To keep it simple right now : Just ask all of them?

gdanezis · 2022-01-07T17:09:50Z

fastpay_core/src/client.rs

+        gas_payment: ObjectRef,
+        module: Vec<u8>,
+    ) -> Result<CertifiedOrder, anyhow::Error> {
+        let module_bytes = vec![module];


Is this not already a vec? Do we need a vec<vec> here?

gdanezis · 2022-01-07T17:11:05Z

fastpay_core/src/client.rs

+        let new_sent_certificates = self
+            .communicate_transfers(
+                self.address,
+                *order.object_id(),


this is a place where we should not have object_id

gdanezis · 2022-01-07T17:11:26Z

fastpay_core/src/client.rs

+        if with_confirmation {
+            self.communicate_transfers(
+                self.address,
+                *order.object_id(),


Here as well this object_id makes no sense now.

gdanezis · 2022-01-07T17:11:54Z

fastpay_core/src/client.rs

+                self.address,
+                *order.object_id(),
+                self.sent_certificates.clone(),
+                CommunicateAction::SynchronizeNextSequenceNumber(


Are these seq numbers per object, otherwise they are also legacy.

gdanezis · 2022-01-07T17:12:41Z

fastpay_core/src/client.rs

+        Box::pin(self.publish(
+            (
+                gas_payment,
+                self.next_sequence_number(gas_payment),


Should this be the next, or the current sequence number?

fastpay/src/client.rs

Somehow this option did not show up in my autocomplete so I thought you made a typo Co-authored-by: Sam Blackshear <sam@mystenlabs.com>

sblackshear · 2022-01-11T21:38:02Z

fastpay/src/client.rs

+                ..Default::default()
+            };
+            let package = build_cfg
+                .compile_package(&path, &mut Vec::new())


This will compile just fine, but unfortunately the compiled modules will fail to link against anything that's already published on-chain. That is, the package might have something like:

module MyPackage::MyModule { use FastX::ID; }

. Compiling this code will produce a module handle 0x2::ID, but the on-chain handle for that type is 0x3C2B307C3239F61643AF5E9A09D7D0C9::ID

We need to do something like https://github.com/MystenLabs/fastnft/blob/main/fastx_programmability/adapter/src/adapter.rs#L186, but that rewrites handles referring to modules already published on-chain.

CC @lxfind to double-check that this makes sense.

@sblackshear We do it here during module publishing in the adapter: https://github.com/MystenLabs/fastnft/blob/main/fastx_programmability/adapter/src/adapter.rs#L156
right?

Ah ok the FastX framework is special in the sense that it has a built-in address which is 0x2 in the move.toml, but with a different address on-chain. So for any modules being published, we first need to remap all of 0x2 to the right on-chain address. But this is specific to 0x2, right? For any new addresses, they would be fine because devs don't define their own address?

Yes, I think this should be specific to 0x2 in the short term.

In the longer term, there's a thorny question of how to write a package P1 such that another user-defined package P2 can easily depend on it. The two options seem to be

P1 follows the FastX framework style and writes every module as module P1::MyModule. We'll then need to have the client understand and/or discover how to rewrite each P1 as well.

P1 directly uses the ObjectID's of its modules -- e.g. module MyModule::MyModule + add MyModule = 0x2fe5ca... in Move.toml (and we could also refactor the framework to do this if desired). The problem with this approach is it's somewhat annoying as a dev--you will have to declare a new Move.toml symbolic address for every module and you won't be able to update it to its final value until you've actually published the package. Maybe we could have the client tooling fill in the symbolic address for you in the package manifest before compiling/publishing, though.

I have a slight preference for (1) because I think it will be easier to use Move packages from elsewhere that don't follow the "one address per module" convention (e.g., the Move stdlib!) but needs more thought/discussion...

sblackshear · 2022-01-12T01:21:29Z

fastpay/src/client.rs

+}
+
+fn find_owner_by_object_cache(
+    account_config: &mut AccountsConfig,


Suggested change

account_config: &mut AccountsConfig,

account_config: &AccountsConfig,

sblackshear · 2022-01-12T01:26:13Z

fastpay/src/client.rs

+            gas_budget,
+        } => {
+            // Find owner of acc
+            let owner = find_owner_by_object_cache(&mut accounts_config, gas_object_id)


This seems ok for now, but a TODO to add a designated owner field to AccountConfig might be good.

sblackshear · 2022-01-12T01:29:03Z

fastpay/src/client.rs


+        // TODO: this somehow turns into a Vec<Vec<u8>>, how?
+        /// Pure args
+        //pure_args: String,


Try

#[structopt(long = "args", parse(try_from_str = parser::parse_transaction_argument))] args: Vec<TransactionArgument>,

here. Then, you can use https://github.com/diem/diem/blob/main/language/move-core/types/src/transaction_argument.rs#L75 to turn it into a Vec<Vec<u8>>

sblackshear · 2022-01-12T01:29:17Z

fastpay/src/client.rs

+
+        // // TODO: object args from
+        // /// Pure args
+        // object_args: String,


Suggested change

// object_args: String,

huitseeker · 2022-01-12T15:42:21Z

fastpay/src/client.rs

+    for acc in account_config.accounts() {
+        if acc.object_ids.contains_key(&object_id) {
+            return Some(acc.address);
+        }
+    }


Consider

for acc in account_config.accounts() if acc.object_ids.contains_key(&object_id) { return Some(acc.address); }

or

account_config.accounts().find_map(|acc| { if acc.object_ids.contains_key(&object_id)) { Some(acc.address); } else { None } }

huitseeker · 2022-01-12T15:53:09Z

fastpay/src/client.rs

+                    // Fetch the obj ref
+                    let obj_info_req = ObjectInfoRequest {
+                        object_id: gas_object_id,
+                        request_sequence_number: None,
+                        request_received_transfers_excluding_first_nth: None,
+                    };
+
+                    let gas_obj_info = client_state.get_object_info(obj_info_req).await.unwrap();
+                    object_args_refs.push(gas_obj_info.object.to_object_reference());


Since there is no loop index, those values are rebuilt identically on every iteration. Is that really what you want?

huitseeker · 2022-01-12T15:55:32Z

fastpay_core/src/client.rs

+            .clone()
+            .unwrap()


In general, you want to unwrap before you clone, or use cloned. Not that you need to clone here (see the into_iter comment).

huitseeker · 2022-01-12T15:56:10Z

fastpay_core/src/client.rs

+            .effects
+            .mutated
+        {
+            self.object_ids.insert(obj_id, seq_no);


... and more generally, this is the line where I would expect to see the clone if there needed to be one. Not that you need to clone here (see the into_iter comment).

huitseeker · 2022-01-12T15:56:38Z

fastpay_core/src/client.rs

@@ -579,6 +705,55 @@ where
        Ok(())
    }

+    fn update_objects_from_order_info(&mut self, order_info_resp: OrderInfoResponse) {
+        // TODO: use the digest also
+        for (obj_id, seq_no, _) in order_info_resp


You can iterate on values directly with into_iter.

huitseeker · 2022-01-12T15:59:22Z

fastpay_core/src/client.rs

@@ -579,6 +705,55 @@ where
        Ok(())
    }

+    fn update_objects_from_order_info(&mut self, order_info_resp: OrderInfoResponse) {


In this function, are order_info_resp.signed_effects.unwrap() instances really representative of an irrecoverable error that should crash the node?

huitseeker · 2022-01-12T16:02:23Z

fastpay_core/src/client.rs

+        self.update_certificates(vec![new_certificate.clone()])?;
+
+        // Confirmation
+        let order_info = self
+            .communicate_confirmation_order(&new_certificate)
+            .await?;


This pattern is repeated multiple time: note that if it makes sense to communicate the new certificate before the local update, you can avoid a clone (if it doesn't, it's not a big deal).

oxade · 2022-01-14T16:46:54Z

Handling Calls separately while publish system is being worked out
Calls PR here #157

Basic object publishing from CLI. Still buggy

c451925

oxade requested review from huitseeker, lxfind, patrickkuo, sblackshear, gdanezis and kchalkias January 7, 2022 01:29

oxade marked this pull request as draft January 7, 2022 01:29

oxade changed the title ~~Basic object publishing from CLI. Still buggy~~ Basic object publishing from CLI. Jan 7, 2022

oxade changed the title ~~Basic object publishing from CLI.~~ Basic module publishing from CLI. Jan 7, 2022

oxade commented Jan 7, 2022

View reviewed changes

oxade added 3 commits January 7, 2022 09:20

Synced to main

8e03754

fixed conflicts

0f1516d

Basic syncing in client

97dc390

gdanezis reviewed Jan 7, 2022

View reviewed changes

oxade added 15 commits January 8, 2022 13:34

sync_to_all_auth and publish comms skeletons

89e4ba4

Merge branch 'main' into feature/publish-module-from-client

ab16d9b

Merge branch 'main' into feature/publish-module-from-client

5a5da1f

Merge branch 'main' into feature/publish-module-from-client

73583a1

Merge branch 'main' into feature/publish-module-from-client

b4c6a1c

Get object owner impl

4097baa

Fetch owner by obj

8eac142

Fetch owner for transfer

069a1d5

Removed owner fetch

642c7c1

Merge branch 'main' into feature/publish-module-from-client

b735dc1

Added publishing logic

24217cc

Added module compile and reporting

44d1434

Merge branch 'main' into feature/publish-module-from-client

5a3ce73

Fix tokio versioning mut change

2f37661

updated readme

f58f530

oxade added 2 commits January 11, 2022 15:36

Added call support and extended object info resp

aaa8617

Merge branch 'main' into feature/publish-module-from-client

1f5caa4

oxade changed the title ~~Basic module publishing from CLI.~~ Basic module publishing and move calls from CLI. Jan 11, 2022

sblackshear reviewed Jan 11, 2022

View reviewed changes

fastpay/src/client.rs Outdated Show resolved Hide resolved

fastpay/src/client.rs Outdated Show resolved Hide resolved

fastpay/src/client.rs Outdated Show resolved Hide resolved

fastpay/src/client.rs Outdated Show resolved Hide resolved

oxade added 2 commits January 11, 2022 16:17

Addressed comments

23c68f1

Clippy and fmt

afba4c7

sblackshear reviewed Jan 11, 2022

View reviewed changes

fastpay/src/client.rs Outdated Show resolved Hide resolved

Update fastpay/src/client.rs

7b39aac

Somehow this option did not show up in my autocomplete so I thought you made a typo Co-authored-by: Sam Blackshear <sam@mystenlabs.com>

sblackshear reviewed Jan 11, 2022

View reviewed changes

sblackshear reviewed Jan 12, 2022

View reviewed changes

huitseeker reviewed Jan 12, 2022

View reviewed changes

oxade closed this Jan 14, 2022

oxade deleted the feature/publish-module-from-client branch January 22, 2022 23:07

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

Simplify subscriber handle for single client (#133)

0cfbe23

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

Simplify subscriber handle for single client (MystenLabs#133)

0bc5928

	account_config: &mut AccountsConfig,
	account_config: &AccountsConfig,

Basic module publishing and move calls from CLI. #133

Basic module publishing and move calls from CLI. #133

Uh oh!

Conversation

oxade commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdanezis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sblackshear Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sblackshear Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oxade commented Jan 14, 2022

Uh oh!

Uh oh!

oxade commented Jan 7, 2022 •

edited

Loading

sblackshear Jan 11, 2022 •

edited

Loading

sblackshear Jan 11, 2022 •

edited

Loading