-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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
[txn-emitter][simple] Reduce flakiness in txn emitter #14299
Conversation
⏱️ 21m total CI duration on this PR
|
18c8930
to
63504ff
Compare
crates/aptos-rest-client/src/lib.rs
Outdated
pub async fn view_account_balance( | ||
&self, | ||
address: AccountAddress, | ||
coin_type: &str, | ||
) -> AptosResult<Response<u64>> { | ||
let resp = self | ||
.view(&ViewRequest { | ||
function: "0x1::coin::balance".parse()?, | ||
type_arguments: vec![coin_type.parse()?], | ||
arguments: vec![serde_json::Value::String(address.to_hex_literal())], | ||
}, None) | ||
.await?; | ||
|
||
resp.and_then(|result| { | ||
if result.len() != 1 { | ||
Err(anyhow!("Wrong data size returned: {:?}", result).into()) | ||
} else { | ||
result[0].as_str() | ||
.ok_or_else(|| anyhow!("Wrong data type returned: {:?}", result).into()) | ||
.and_then(|v| v.parse::<u64>().map_err(|_e| anyhow!("Couldn't parse returned value: {:?}", result).into())) | ||
} | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would replace the existing balance call, with the BCS version since it's all abstracted away. Don't need two of these as view function functions, on top of the existing function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
view might be more expensive, so wanted it to be explicit.
but I can just rename from get_account_balance to view_account_balance and have all callers updated.
5bc7bb5
to
3f36d79
Compare
@gregnazario updated , does it look good now? I couldn't run your test, as I don't have your account configuration locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#seems-legit
would be nice if PR is with a proper summary.
.collect::<Vec<_>>() | ||
.await | ||
.into_iter() | ||
.collect::<Result<Vec<_>, _>>()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use try_collect()
?
3f36d79
to
2607526
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2607526
to
8c92e95
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8c92e95
to
c5a9a7e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Key Areas to Review
Checklist