Skip to content
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: add http_request_next method (large asset support) #129

Merged
merged 7 commits into from
Mar 19, 2021

Conversation

ericswanson-dfinity
Copy link
Member

@ericswanson-dfinity ericswanson-dfinity commented Mar 17, 2021


#[derive(CandidType, Deserialize)]
pub struct HttpGetChunkResponse {
pub chunk: Vec<u8>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub chunk: Vec<u8>,
#[serde(with = "serde_bytes")]
pub chunk: Vec<u8>,

@@ -29,6 +29,18 @@ pub struct HttpResponse {
pub body: Vec<u8>,
}

#[derive(CandidType)]
pub struct HttpGetChunkRequest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be moved inside the body of http_get_chunk similar to https://github.com/dfinity/agent-rs/blob/next/ic-utils/src/interfaces/management_canister.rs#L453

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need design. See dfinity/dx-triage#126 where I'll start the discussion.

@ericswanson-dfinity ericswanson-dfinity changed the title feat: add http_get_chunk method (large asset support) feat: add http_request_next method (large asset support) Mar 18, 2021
@@ -192,6 +193,25 @@ impl Argument {
}
}

/// Add an IDLValue Argument. If the current value of Argument is Raw, will set the
/// result to an error. If the current value is an error, will do nothing.
pub fn push_value_arg(&mut self, arg: IDLValue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDLValue implements CandidType, so you should be able to use push_idl_arg with an IDLValue. Was there any issue with using that other function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -258,6 +278,16 @@ impl<'agent, 'canister: 'agent, Interface> SyncCallBuilder<'agent, 'canister, In
self
}

/// Add an argument to the candid argument list. This requires Candid arguments, if
/// there is a raw argument set (using [with_arg_raw]), this will fail.
pub fn with_value_arg(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above.

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay LGTM, but this is an anti-pattern. We might have to deprecate these methods if we make IDLValue work.

@ericswanson-dfinity
Copy link
Member Author

Entered #132 to make with_arg work with IDLValue.

@ericswanson-dfinity ericswanson-dfinity merged commit cd9632e into next Mar 19, 2021
@ericswanson-dfinity ericswanson-dfinity deleted the ericswanson/126-http-server-chunked-assets branch March 19, 2021 00:27
mergify bot pushed a commit to dfinity/sdk that referenced this pull request Mar 22, 2021
1. The `http_request` response now includes an optional `next_token` field if there are more chunks after the first
2. The `dfx` built-in webserver calls `http_request_next` to get subsequent chunks


Requires dfinity/agent-rs#129
Implements dfinity/dx-triage#126
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants