-
Notifications
You must be signed in to change notification settings - Fork 82
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
feat: response authentication #1197
Conversation
seems to fix stuff for me
src/dfx/src/lib/root_key.rs
Outdated
use anyhow::anyhow; | ||
|
||
pub async fn fetch_root_key_if_needed<'a>(env: &'a (dyn Environment + 'a)) -> DfxResult { | ||
let must_fetch_root_key = env |
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.
maybe rename to non_ic_network
or something?
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.
Thanks, naming is hard haha
src/dfx/src/lib/root_key.rs
Outdated
if must_fetch_root_key { | ||
let agent = env | ||
.get_agent() | ||
.ok_or_else(|| anyhow!("Cannot get HTTP client from environment."))?; |
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 guess this error message was changed when moving to anyhow
, it seems like it should be what CommandMustBeRunInAProject
was. This error doesn't really help a user figure out what is the right environment
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 copied this from other code that gets an agent. This is one of those errors that can't actually happen, too, because the Environment
is actually always an AgentEnvironment
.
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.
Really I think Environment::get_agent
should panic and the return type should be Agent
not Option<Agent>
@@ -104,6 +105,9 @@ pub fn exec(env: &dyn Environment, opts: CanisterCallOpts) -> DfxResult { | |||
.get_agent() | |||
.ok_or_else(|| anyhow!("Cannot get HTTP client from environment."))?; | |||
let mut runtime = Runtime::new().expect("Unable to create a runtime"); | |||
|
|||
runtime.block_on(fetch_root_key_if_needed(env))?; |
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.
should we lessen duplication and make all canister subcommand functions async
, then we can move creating the tokio runtime and fetch_root_key_if_needed
to: https://github.com/dfinity/sdk/blob/master/src/dfx/src/commands/canister/mod.rs#L45
(just a thought, not something actionable for this pr)
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.
Yeah, that is a tech debt item, to make all command exec
functions async. I didn't think we could do it only for the canister commands, though maybe it is possible?
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.
lgtm!
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.
Blocking this because fetch_root_key
needs to always be called now.
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.
@ericswanson-dfinity approved offline (can't approve your own PR). Unblocking this.
old school 👍 |
## Changelog for advisory-db: Branch: main Commits: [rustsec/advisory-db@97388358...66275561](rustsec/advisory-db@9738835...6627556) * [`52b96a91`](rustsec/advisory-db@52b96a9) Add unsoundness advisory for enum-map ([RustSec/advisory-db#1197](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/1197)) * [`66275561`](rustsec/advisory-db@6627556) Assigned RUSTSEC-2022-0010 to enum-map ([RustSec/advisory-db#1198](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/1198))
## Changelog for advisory-db: Branch: main Commits: [rustsec/advisory-db@97388358...66275561](rustsec/advisory-db@9738835...6627556) * [`52b96a91`](rustsec/advisory-db@52b96a9) Add unsoundness advisory for enum-map ([RustSec/advisory-db#1197](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/1197)) * [`66275561`](rustsec/advisory-db@6627556) Assigned RUSTSEC-2022-0010 to enum-map ([RustSec/advisory-db#1198](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/1198))
ic