Fix certificate update issue which cause subsequence object transfer fail #170

patrickkuo · 2022-01-13T18:11:04Z

added extensive test to make sure certificate updates correctly on client after transfer
merged sent_certificates and received_certificates
This PR address issues [fastx client] Key Certificates By ObjectRef #82 and [fastx client] Update Certificates For Multi Object Orders #173

patrickkuo · 2022-01-16T17:51:32Z

This PR is ready for review, I will add more client side test for move calls in my next PR.

gdanezis

This is good.

I only have one architectural issue here: the client seems to not remember which certificates are "pending". These are certificates that the client knows BUT have not been confirmed by 2f+1 authorities. We should probably remember these, and re-submit them on restart of the client, or retry to send them if the network goes down. Most importantly the client must NEVER reuse the sequence number used in an order that has a signature from an authority.

gdanezis · 2022-01-17T13:27:29Z

fastpay/src/config.rs

@@ -235,12 +232,12 @@ impl AccountsConfig {
                if let Address::FastPay(recipient) = &transfer.recipient {
                    if let Some(config) = self.accounts.get_mut(recipient) {
                        if let Err(position) = config
-                            .received_certificates
+                            .certificates
                            .binary_search_by_key(&certificate.order.digest(), |cert| {


Why not store the certificates in a HashMap/Btree sorted by digest instead?

patrickkuo · 2022-01-18T13:47:47Z

The client query the authorities and confirm all the missing certificates at the beginning of communicate_transfers https://github.com/MystenLabs/fastnft/blob/178a9fb289db3222fb4bf51208501513f13d41b5/fastpay_core/src/client.rs#L448
I think this is sufficient to prevent "pending" certificates?

huitseeker

Mostly nits, one comment on update_certificates

huitseeker · 2022-01-18T14:29:41Z

fastpay_core/src/authority.rs

+        let requested_certificate = match request.request_sequence_number {
+            Some(seq) => {
+                // TODO(https://github.com/MystenLabs/fastnft/issues/123): Here we need to develop a strategy
+                // to provide back to the client the object digest for specific objects requested. Probably,
+                // we have to return the full ObjectRef and why not the actual full object here.
+
+                // Get the Transaction Digest that created the object
+                let parent_iterator = self
+                    .get_parent_iterator(request.object_id, Some(seq.increment()?))
+                    .await?;
+                let (_, transaction_digest) = parent_iterator
+                    .first()
+                    .ok_or(FastPayError::CertificateNotfound)?;
+                // Get the cert from the transaction digest
+                Some(
+                    self.read_certificate(transaction_digest)
+                        .await?
+                        .ok_or(FastPayError::CertificateNotfound)?,
+                )
+            }
+            None => None,
+        };


Nit: you're only doing something non-trivial with one arm of the match, so that's a good place to reformulate your :

let rc = match foo { Some(bar) => { ... Some(baz) } None => None }

Into:

let rc = { if let Some(bar) = foo { ... Some(baz) } else { None } }

Or even

let rc = foo.map(|baz| { ... baz }

my first attempt was .map, but couldn't get it working because of async and ?, are there any way to make it work?

I was just mentioning it for completeness. It's actually hard, though not impossible, to work with a map in contexts where you're like to short-circuit with a ?, you have to keep things as a Result, thread operations through flat_map/map, extirpate your Result out of whatever you're returning, and only ? or return at the end.

Sometimes the if let construct is just simpler.

huitseeker · 2022-01-18T14:31:44Z

fastpay_core/src/client.rs

+                .filter(|(id, _, _)| id == &object_id)
+                .map(|(_, seq, _)| seq)


Nit: consider

Suggested change

.filter(|(id, _, _)| id == &object_id)

.map(|(_, seq, _)| seq)

.filter_map(|(id, seq, _)| if id == &object_id { Some(seq) } else { None })

?

huitseeker · 2022-01-18T14:33:54Z

fastpay_core/src/client.rs

+                .find(|(id, _, _)| object_id == id)
+                .copied()
+                .ok_or::<FastPayError>(FastPayError::ObjectNotFound)
+                .unwrap();


Are you looking for a ? instead of the panicking .unwrap()?

huitseeker · 2022-01-18T14:36:51Z

fastpay_core/src/client.rs

@@ -920,15 +935,15 @@ where
            let transfer = Transfer {
                object_ref: (
                    object_id,
-                    self.next_sequence_number(object_id),
+                    self.next_sequence_number(&object_id).unwrap(),


You're returning a Result here, is the panic appropriate?

huitseeker · 2022-01-18T14:36:56Z

fastpay_core/src/client.rs

                    // TODO(https://github.com/MystenLabs/fastnft/issues/123): Include actual object digest here
                    ObjectDigest::new([0; 32]),
                ),
                sender: self.address,
                recipient: Address::FastPay(recipient),
                gas_payment: (
                    gas_payment,
-                    self.next_sequence_number(gas_payment),
+                    self.next_sequence_number(&gas_payment).unwrap(),


Same Q: You're returning a Result here, is the panic appropriate?

huitseeker · 2022-01-18T14:46:19Z

fastpay_core/src/client.rs

                ),
            )
            .await?;
        }
-        Ok(self.sent_certificates.last().unwrap().clone())
+        Ok(self.certificates(order.object_id()).last().unwrap().clone())


Nit: A comment on the safety of last().unwrap() somewhere in this function, one that would refer to update_certificates only growing the set, should help the next implementer reason locally.

…ause by incorrect certificate update. added extensive test to check the certificate update logic.

some code clean up

…ransactionDigest, CertifiedOrder>

huitseeker

thanks!

patrickkuo force-pushed the pat/client-side-cert branch 2 times, most recently from 7a932f1 to 89ad047 Compare January 14, 2022 10:44

patrickkuo mentioned this pull request Jan 14, 2022

[fastx client] Update Certificates For Multi Object Orders #173

Closed

patrickkuo force-pushed the pat/client-side-cert branch 2 times, most recently from 8d13173 to 84f38ba Compare January 16, 2022 17:34

patrickkuo marked this pull request as ready for review January 16, 2022 17:50

patrickkuo requested review from gdanezis, oxade, huitseeker, sblackshear, awelc and lxfind January 16, 2022 17:51

patrickkuo force-pushed the pat/client-side-cert branch from 101187c to 1e59f57 Compare January 16, 2022 20:33

gdanezis approved these changes Jan 17, 2022

View reviewed changes

huitseeker reviewed Jan 18, 2022

View reviewed changes

patrickkuo added 7 commits January 18, 2022 17:57

combine received cert and sent cert

049d86c

fix bugs preventing transfer of object after the first transaction, c…

1b43755

…ause by incorrect certificate update. added extensive test to check the certificate update logic.

fix clippy

b986394

use proper error propagation instead of unwrap

ea6d472

use iterator in certificates function instead of Vec

17105ae

some code clean up

address PR comment: changed certificates in UserAccount to BTreeMap<T…

eb95e78

…ransactionDigest, CertifiedOrder>

address PR comment : replaced incorrect unwrap with ?

01094ce

patrickkuo force-pushed the pat/client-side-cert branch from 9d9c2fe to 01094ce Compare January 18, 2022 17:57

huitseeker approved these changes Jan 18, 2022

View reviewed changes

patrickkuo merged commit 5a946cd into main Jan 18, 2022

patrickkuo deleted the pat/client-side-cert branch January 24, 2022 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix certificate update issue which cause subsequence object transfer fail #170

Fix certificate update issue which cause subsequence object transfer fail #170

patrickkuo commented Jan 13, 2022 •

edited

Loading

patrickkuo commented Jan 16, 2022

gdanezis left a comment

gdanezis Jan 17, 2022

patrickkuo commented Jan 18, 2022 •

edited

Loading

huitseeker left a comment

huitseeker Jan 18, 2022

patrickkuo Jan 18, 2022

huitseeker Jan 18, 2022

huitseeker Jan 18, 2022

huitseeker Jan 18, 2022

huitseeker Jan 18, 2022

huitseeker Jan 18, 2022

huitseeker Jan 18, 2022

huitseeker left a comment

		.filter(\|(id, _, _)\| id == &object_id)
		.map(\|(_, seq, _)\| seq)

	.filter(\|(id, _, _)\| id == &object_id)
	.map(\|(_, seq, _)\| seq)
	.filter_map(\|(id, seq, _)\| if id == &object_id { Some(seq) } else { None })

Fix certificate update issue which cause subsequence object transfer fail #170

Fix certificate update issue which cause subsequence object transfer fail #170

Conversation

patrickkuo commented Jan 13, 2022 • edited Loading

patrickkuo commented Jan 16, 2022

gdanezis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickkuo commented Jan 18, 2022 • edited Loading

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

huitseeker left a comment

Choose a reason for hiding this comment

patrickkuo commented Jan 13, 2022 •

edited

Loading

patrickkuo commented Jan 18, 2022 •

edited

Loading