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

[fastx client] TX Operations From CLI Fail #316

Merged
merged 3 commits into from
Feb 2, 2022
Merged

[fastx client] TX Operations From CLI Fail #316

merged 3 commits into from
Feb 2, 2022

Conversation

patrickkuo
Copy link
Contributor

  • Fix cli errors due to checks of actual object digest in authorities

    • create account now export full object to file

  • Fixed bug in benchmark caused by change of TransferOrder's response, transfer now returns OrderInfoResponse

this PR fixes #277

@patrickkuo patrickkuo self-assigned this Jan 30, 2022
@patrickkuo patrickkuo requested a review from oxade January 30, 2022 23:08
@patrickkuo patrickkuo added this to the Milestone 3 milestone Jan 30, 2022
huitseeker
huitseeker reviewed Feb 1, 2022
View reviewed changes
fastpay/src/config.rs
let (address, key) = get_key_pair();
pub fn new(
address: FastPayAddress,
key: KeyPair,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have an issue opened to replace this with a trait that happens to provide the sign public_key, verify functions we might need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add that in together with refactoring of the CLI, #341

fastpay/src/client.rs
Comment on lines +806 to +839
value_per_obj,
initial_state_config_path,
} => {
let num_accounts: u32 = num;
let num_of_addresses: u32 = num;
let mut init_state_cfg: InitialStateConfig = InitialStateConfig::new();

for _ in 0..num_accounts {
for _ in 0..num_of_addresses {
let (address, key) = get_key_pair();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not own the public key assignment for those accounts, it should be left to the user to provide public keys (or better, addresses) for ownership ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these code are for demo only, not intended for production use, I will move it out of CLI client in this issue #341

@patrickkuo
* Fix cli bug due to checks of actual object digest in authorities
acd7be7 
* Fixed bug in benchmark caused by change of TransferOrder's response, transfer now returns OrderInfoResponse
@patrickkuo
minor fixes
0c5fd99
huitseeker
huitseeker approved these changes Feb 2, 2022
View reviewed changes
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Mostly nits, thanks!

fastpay/src/client.rs
Comment on lines +802 to +809
Some(OrderInfoResponse {
signed_order: Some(order),
..
}) => {
confirmed.insert(*order.order.object_id());
acc + 1
}
None => acc,
_ => acc,
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice 🤩

fastpay/src/server.rs Outdated
Comment on lines 53 to 56
for initial_state_cfg_entry in &initial_accounts_config.config {
let address = &initial_state_cfg_entry.address;
for (object_id, _, _) in &initial_state_cfg_entry.object_refs {
let object = Object::with_id_owner_for_testing(*object_id, *address);

for object in &initial_state_cfg_entry.objects {
state.init_order_lock(object.to_object_reference()).await;
state.insert_object(object).await;
state.insert_object(object.clone()).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you are in a constructor and own initial_accounts_config, which you created. I think it's a good place to try using into_iter to consume the values in both of these iterators, thereby possibly eliminating the final clone.

@patrickkuo patrickkuo merged commit f458c89 into main Feb 2, 2022
@patrickkuo patrickkuo deleted the pat/cli-bug-fix branch February 2, 2022 15:53
@oxade
Copy link
Contributor

oxade commented Feb 3, 2022 •
edited
Loading

@patrickkuo how did you verify this fix?
I'm getting incorrect signer error

F32A07DCB5AE07497C3E34A288A794D1D60B23F3: (F32A07DCB5AE07497C3E34A288A794D1D60B23F3, SequenceNumber(0), ObjectDigest([144, 63, 48, 43, 75, 144, 224, 121, 98, 27, 160, 249, 50, 111, 233, 97, 29, 157, 213, 23, 226, 243, 101, 62, 7, 49, 118, 27, 56, 114, 235, 113]))
2022-02-03T05:37:54.672903Z  INFO fastpay_core::authority_server: Listening to TCP traffic on 0.0.0.0:9300
2022-02-03T05:37:54.691054Z  INFO fastpay_core::authority_server: Listening to TCP traffic on 0.0.0.0:9100
2022-02-03T05:37:54.696665Z  INFO fastpay_core::authority_server: Listening to TCP traffic on 0.0.0.0:9400
2022-02-03T05:37:54.698579Z  INFO fastpay_core::authority_server: Listening to TCP traffic on 0.0.0.0:9200
2022-02-03T05:37:54.713288Z  INFO client: Starting transfer
2022-02-03T05:37:54.715272Z  WARN fastpay_core::authority_server: User query failed: Error acquiring lock for object(s): [IncorrectSigner]
2022-02-03T05:37:54.715272Z  WARN fastpay_core::authority_server: User query failed: Error acquiring lock for object(s): [IncorrectSigner]
2022-02-03T05:37:54.715272Z  WARN fastpay_core::authority_server: User query failed: Error acquiring lock for object(s): [IncorrectSigner]
2022-02-03T05:37:54.715272Z  WARN fastpay_core::authority_server: User query failed: Error acquiring lock for object(s): [IncorrectSigner]
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Failed to achieve quorum between authorities, cause by : [
    "Error acquiring lock for object(s): [IncorrectSigner]",
]', fastpay/src/client.rs:682:22

@patrickkuo
Copy link
Contributor Author

@oxade this is cause by the path of the client db, https://github.com/MystenLabs/fastnft/blob/da973ca2871717132953d10d6f4f1c572eca44f8/fastpay/src/client.rs#L479-L481

this resolve into the same location every time, the cli will keep adding to the db without removing it for new demo run.
Which causes error in authorities because the object provided are stale.

@oxade
Copy link
Contributor

oxade commented Feb 3, 2022 •
edited
Loading

@oxade this is cause by the path of the client db,

https://github.com/MystenLabs/fastnft/blob/da973ca2871717132953d10d6f4f1c572eca44f8/fastpay/src/client.rs#L479-L481

this resolve into the same location every time, the cli will keep adding to the db without removing it for new demo run. Which causes error in authorities because the object provided are stale.

Right
I forgot the cli is ephemeral and didn't consider improving it
Need to get rid of that logic today
Thanks for looking at it man

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huitseeker huitseeker huitseeker approved these changes

@sblackshear sblackshear sblackshear approved these changes

@oxade oxade Awaiting requested review from oxade

@arun-koshy arun-koshy Awaiting requested review from arun-koshy

@awelc awelc Awaiting requested review from awelc

@gdanezis gdanezis Awaiting requested review from gdanezis

Assignees

@patrickkuo patrickkuo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[fastx client] TX Operations From CLI Fail
4 participants
@patrickkuo @oxade @huitseeker @sblackshear