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: allow upload/retrieve of assets of arbitrary size from asset canister #1482

Merged
merged 52 commits into from
Mar 12, 2021

Conversation

ericswanson-dfinity
Copy link
Member

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

Per the design doc:

  1. Adds asset canister methods:

    • create_batch()
    • create_chunk()
    • commit_batch()
    • get()
    • get_chunk()
  2. Reworks the asset installer in dfx to use these methods. It can therefore upload assets that exceed the message ingress size. Separate work (which this PR enables) will have to update agent-js to download these large assets.

Other than allowing the storage and retrieval of large assets, this PR does not address:

  • multiple content types and content encodings: in this PR dfx always uploads with content-type: application/octet-stream and content-encoding: identity
  • smart/correct synchronization: in this PR dfx always uploads all assets (even those that have not changed), and still does not delete assets that were previously uploaded but no longer exist.
  • All assets and chunks are uploaded in series. See Upload asset chunks concurrently #1491 and Calculate ingress_expiry_datetime closer to update call execution agent-rs#125
  • The store(), retrieve(), and list() method signatures are unchanged for the time being, for backward compatibility. Later work will remove the retrieve() method, change the semantics and parameters of store(), and change the parameters and return type of list().
Uploading assets to asset canister...
  large-asset.bin 1/7 (1900000 bytes)
  large-asset.bin 2/7 (1900000 bytes)
  large-asset.bin 3/7 (1900000 bytes)
  large-asset.bin 4/7 (1900000 bytes)
  large-asset.bin 5/7 (1900000 bytes)
  large-asset.bin 6/7 (1900000 bytes)
  large-asset.bin 7/7 (1100000 bytes)
  index.js 1/1 (1218 bytes)
  sample-asset.txt 1/1 (24 bytes)
  index.js.map 1/1 (5625 bytes)

ericswanson-dfinity and others added 3 commits March 8, 2021 17:37
Use serde_bytes for [u8]

Co-authored-by: Yan Chen <48968912+chenyan-dfinity@users.noreply.github.com>
@p-shahi
Copy link
Contributor

p-shahi commented Mar 10, 2021

Thanks for walking me through these change yesterday! in general it lgtm. I am taking a pass through the .rs sections...
an aside: Do we have ticket to improve expiry timeout computation in agent-rs UpdateBuilder and support for concurrent update calls? Maybe you can link them to this pr

Copy link
Contributor

@p-shahi p-shahi left a comment

Choose a reason for hiding this comment

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

may want review from others as well

mergify bot pushed a commit that referenced this pull request Mar 11, 2021
…#1483)

And add support for http requests in the base storage canister (with a default to `/index.html`).

This does not support other encodings than `identity` for now (and doesn't even return any headers). This support will be added to the upgraded asset storage canister built in #1482.

Added a test that uses `curl localhost` to test that the asset storage AND the webserver properly support the http requests.

This commit also upgrades tokio and reqwest in order to work correctly. There are also _some_ performance issues noted (this is slower than the `icx-http-server` for some reason), but those are not considered criticals and could be improved later on.

Renamed the `project_name` in our own generated assets to `canister_name`, for things that are generated during canister build (and not project generation).

Ref https://github.com/dfinity/dx-triage/issues/96
@mergify mergify bot merged commit 464fd35 into master Mar 12, 2021
@mergify mergify bot deleted the ericswanson/112-asset-canister-large-assets branch March 12, 2021 03:19
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