Remove balance from client test and added object ownership checks in test #48
Conversation
patrickkuo
requested review from
gdanezis,
huitseeker,
lxfind and
sblackshear
patrickkuo
changed the title
There was a problem hiding this comment.
Nice one, and particularly on the tests which were a mess. See my comment on whether you want to return the latest seq number of a given object that is supported by at least some honest node, which I think the client might need for various sync / confirmations.
fastpay_core/src/client.rs
Outdated
| /// NOTE: This is only reliable in the synchronous model, with a sufficient timeout value. | ||
| #[cfg(test)] | ||
| async fn get_strong_majority_balance(&mut self, object_id: ObjectID) -> Balance { | ||
| async fn object_ownership_have_quorum(&mut self, object_id: ObjectID) -> bool { |
There was a problem hiding this comment.
I wonder: is it useful here to return the highest sequence number for which we have a quorum instead of just true/false?
There was a problem hiding this comment.
Good call! this will be much more useful then bool
fastpay_core/src/client.rs
Outdated
| Ok(_info) => Some((*name, Balance::from(0))), | ||
| _ => None, | ||
| Ok(info) => { | ||
| if info.owner == address { |
There was a problem hiding this comment.
I think this test is a skeleton from when we had address as the index. Should it now be something like "info.object_id == object_id". And if you want to keep track of sequence numbers you can store Some(info.seq) to analyze later.
There was a problem hiding this comment.
info.object_id == object_id will always be true in this case, because the object id is use in the Request and the Response should return data for the same object_id.
This check is for checking if the info requester is the owner of the object
There was a problem hiding this comment.
Why does it matter if the info requester is the owner of the object?
There was a problem hiding this comment.
this method is for testing the object have the right ownership after transfer
…m to confirm the client have ownership of an object in test
patrickkuo
force-pushed
the
pat/client_test_refactoring
branch
from
b10ec4f to
20e28f7
Compare
| /// NOTE: This is only reliable in the synchronous model, with a sufficient timeout value. | ||
| #[cfg(test)] | ||
| async fn get_strong_majority_balance(&mut self, object_id: ObjectID) -> Balance { | ||
| async fn get_strong_majority_owner( |
There was a problem hiding this comment.
Nit: doc comment needs updating to reflect new return value
| .try_lock() | ||
| .unwrap() | ||
| .init_order_lock((object_id, 0.into())); | ||
| let object_ids: Vec<ObjectID> = object_ids.next().unwrap_or(Vec::new()); |
There was a problem hiding this comment.
have you considered writing this by doing something like for (client, object_ids) in clients.zip(object_ids)?
| let _balances = balances.into_iter().map(Balance::from); | ||
| for (_, client) in clients.iter_mut() { | ||
| let mut object_ids = object_ids.into_iter(); | ||
| for client in clients.clone() { | ||
| let addr = address; |
There was a problem hiding this comment.
Is this alias meaningful?
There was a problem hiding this comment.
nope, will remove it
| let numbers: futures::stream::FuturesUnordered<_> = self | ||
| .authority_clients | ||
| let mut authority_clients = self.authority_clients.clone(); | ||
| let numbers: futures::stream::FuturesUnordered<_> = authority_clients | ||
| .iter_mut() | ||
| .map(|(name, client)| { | ||
| let fut = client.handle_account_info_request(request.clone()); |
There was a problem hiding this comment.
It's fishy that handle_account_info_request requires a mutable client, and I'd love a bit of insight as to why.
There was a problem hiding this comment.
this is originated from send_recv_bytes_internal in network, which required &mut self unnecessarily, I have changed it to &self in my next check in
…ll functions uses it.
Co-authored-by: François Garillot <[email protected]>
TODO: