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

Create ApplicationDescription blob type #2612

Open
wants to merge 2 commits into
base: graphite-base/2612
Choose a base branch
from

Conversation

ndr-ds
Copy link
Contributor

@ndr-ds ndr-ds commented Oct 12, 2024

Motivation

We need a new ApplicationDescription blob type

Proposal

Add the blob type, but don't use it yet.

Test Plan

CI

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Copy link
Contributor Author

ndr-ds commented Oct 12, 2024

@ndr-ds ndr-ds force-pushed the 10-11-introduce_blob_bytes_function branch 2 times, most recently from dccbffc to 1edf821 Compare October 14, 2024 15:01
Base automatically changed from 10-11-introduce_blob_bytes_function to main October 14, 2024 15:02
@ndr-ds ndr-ds force-pushed the 10-12-create_applicationdescription_blob_type branch 2 times, most recently from 58ee530 to ea2a259 Compare October 14, 2024 19:04
@ndr-ds ndr-ds changed the base branch from main to 10-11-return_multiple_blobs_not_found_at_once October 14, 2024 19:04
@ndr-ds ndr-ds marked this pull request as ready for review October 14, 2024 21:41
@ndr-ds ndr-ds requested review from afck, jvff and ma2bd October 14, 2024 21:41
pub fn new_data(bytes: Vec<u8>) -> Self {
BlobContent::new_data(bytes).into()
pub fn new_data(bytes: Vec<u8>) -> Result<Self, bcs::Error> {
BlobContent::new_data(bytes).try_into()
Copy link
Contributor

@afck afck Oct 15, 2024

Choose a reason for hiding this comment

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

That really can't fail, can it? Would it make sense to expect here? (And in the other methods that can't fail?)
Or even inline the relevant part of the implementation, so the Result doesn't show up in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if it fails, something is very wrong 😅 so it might make sense to panic. I'll change it!

Copy link
Contributor

@afck afck left a comment

Choose a reason for hiding this comment

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

Approving, but maybe let's wait with merging this until the follow-up PR is also approved, so we don't have those duplicated types on main for too long.

@ndr-ds ndr-ds force-pushed the 10-11-return_multiple_blobs_not_found_at_once branch from 73f3541 to cd0f531 Compare October 16, 2024 00:59
@ndr-ds ndr-ds force-pushed the 10-12-create_applicationdescription_blob_type branch 2 times, most recently from 91372cb to 7fb263d Compare October 16, 2024 01:20
@ndr-ds ndr-ds force-pushed the 10-11-return_multiple_blobs_not_found_at_once branch from cd0f531 to 18622a1 Compare October 16, 2024 03:24
Base automatically changed from 10-11-return_multiple_blobs_not_found_at_once to main October 16, 2024 03:26
@ndr-ds ndr-ds force-pushed the 10-12-create_applicationdescription_blob_type branch from 7fb263d to b3b01c9 Compare October 16, 2024 03:26
@ndr-ds ndr-ds linked an issue Oct 17, 2024 that may be closed by this pull request
@ndr-ds ndr-ds force-pushed the 10-12-create_applicationdescription_blob_type branch from b3b01c9 to f8b54d6 Compare October 18, 2024 02:46
@ndr-ds ndr-ds force-pushed the 10-12-create_applicationdescription_blob_type branch from f8b54d6 to 38bd265 Compare October 18, 2024 20:31
@ndr-ds ndr-ds changed the base branch from main to 10-18-blobnotfoundonread_-_blobsnotfound October 18, 2024 20:31
@@ -142,6 +142,8 @@ pub enum ExecutionError {
JoinError(#[from] linera_base::task::Error),
#[error(transparent)]
DecompressionError(#[from] DecompressionError),
#[error(transparent)]
BcsError(#[from] bcs::Error),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for moving this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I removed it then added it somewhere else by accident as the PR evolved. Will fix

Comment on lines 2571 to 2720
let mut blobs = Vec::new();
for byte_vec in bytes {
blobs.push(Blob::new_data(byte_vec));
}
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
let mut blobs = Vec::new();
for byte_vec in bytes {
blobs.push(Blob::new_data(byte_vec));
}
let blobs = bytes.into_iter().map(Blob::new_data).collect::<Vec<_>>();

would allocate the right vector size right away.

@ndr-ds ndr-ds force-pushed the 10-18-blobnotfoundonread_-_blobsnotfound branch from b49501f to 78912fd Compare October 23, 2024 12:23
@ndr-ds ndr-ds force-pushed the 10-12-create_applicationdescription_blob_type branch from 38bd265 to e7a9b98 Compare October 23, 2024 12:29
@ndr-ds ndr-ds force-pushed the 10-18-blobnotfoundonread_-_blobsnotfound branch from 78912fd to bb341a0 Compare October 23, 2024 13:19
@ndr-ds ndr-ds force-pushed the 10-12-create_applicationdescription_blob_type branch from e7a9b98 to ae67606 Compare October 23, 2024 13:26
@ndr-ds ndr-ds force-pushed the 10-18-blobnotfoundonread_-_blobsnotfound branch from bb341a0 to 84566e6 Compare November 5, 2024 21:47
@ndr-ds ndr-ds force-pushed the 10-18-blobnotfoundonread_-_blobsnotfound branch from 84566e6 to ca23cb5 Compare November 7, 2024 00:08
@ndr-ds ndr-ds force-pushed the 10-12-create_applicationdescription_blob_type branch from ae67606 to 7abc4b3 Compare November 7, 2024 02:01
Comment on lines +2717 to +2720
let mut blobs = Vec::with_capacity(bytes.len());
for byte_vec in bytes {
blobs.push(Blob::new_data(byte_vec));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

(Maybe more concise?)

Suggested change
let mut blobs = Vec::with_capacity(bytes.len());
for byte_vec in bytes {
blobs.push(Blob::new_data(byte_vec));
}
let blobs = bytes.into_iter().map(Blob::new_data).collect::<Vec<_>>();

pub creator_chain_id: ChainId,
/// Height of the block that created this application.
pub block_height: BlockHeight,
/// At what efffect index of the block this application was created.
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
/// At what efffect index of the block this application was created.
/// At what effect index of the block this application was created.


impl<A: Ord> Ord for BlobApplicationId<A> {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
(self.application_description_hash, self.bytecode_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we destructure in these methods, the compiler will warn us in case we add any additional fields.

/// Loads data blob content from a file.
pub async fn load_data_blob_from_file(path: impl AsRef<Path>) -> io::Result<Self> {
pub async fn load_data_blob_from_file(path: impl AsRef<Path>) -> Result<Self, io::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Why?)

@ndr-ds ndr-ds force-pushed the 10-18-blobnotfoundonread_-_blobsnotfound branch 3 times, most recently from e9a3222 to b550792 Compare November 13, 2024 19:13
@ndr-ds ndr-ds changed the base branch from 10-18-blobnotfoundonread_-_blobsnotfound to graphite-base/2612 November 13, 2024 20:58
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.

Replace ApplicationDescription by blobs
2 participants