Implement revoke admin cli support #83
Conversation
cli/src/utils.rs
Outdated
|
|
||
| info!("estimated grant gas cost {:#?}", claim_tx.tx.gas().unwrap()); | ||
|
|
||
| send_tx(&claim_tx.tx, client, retries).await?; |
There was a problem hiding this comment.
is it possible for this to throw errors? I don't know rust, but is there some mechanism for handling potential transaction failures? at the very least I think we should add a log line here confirming that the transaction was executed successfully, and ideally point the user at the chain explorer where they can manually verify the address is no longer an admin.
There was a problem hiding this comment.
Yes. The ? operator is going to propagate any error back to the caller function because this function has a Result<(), Box<dyn std::error::Error>> return type.
That's a great suggestion. It would definitely help in terms of usability. I'll look into it :)
Thank you! I agree we should have this. I will have to learn how to use the test tooling to pull it off (which is probably a good thing anyway). Should it be a necessary to merge the PR? |
joaosa
force-pushed
the
implement_cli_revoke_admin
branch
from
4679729
to
49fdbdc
Compare
joaosa
force-pushed
the
implement_cli_revoke_admin
branch
from
49fdbdc
to
1fb33dc
Compare
I should also note that as things are, it's not possible to run integration tests locally as we're missing three secrets and we have to rely on CI. However, it's possible to troubleshoot what's going on with all these invocations on calibrationnet. Here's an example of the last CI run with all the integration test transactions (you'll need the abi.json to decode the messages). |
joaosa
force-pushed
the
implement_cli_revoke_admin
branch
from
eb1fbc4
to
4eb13a1
Compare
We couldn't really remove admins from the cli, so this should fix that. I wanted to cleanup the code a little bit, but I figured it would be more important to ship this rather than adjust existing functionality for code reuse.
revoke_adminmethod;