style!: cleanup blob struct #18
Conversation
| ); | ||
| assert!(blob.is_padded(), "has to be padded"); | ||
|
|
||
| blob.remove_padding().unwrap(); |
There was a problem hiding this comment.
keep the test to revert it back?
There was a problem hiding this comment.
You mean to create a blob and then destruct it getting the raw data back? This test does this, is this what you meant?
| /// if the data has 32 byte segments exceeding the modulo of the field | ||
| /// then the bytes will be modded by the order of the field and the data | ||
| /// will be transformed incorrectly | ||
| pub fn from_padded_bytes_unchecked(input: &[u8]) -> Self { |
There was a problem hiding this comment.
Need to go through the rest of the changes but this function is used in nitro proofs. So will be a breaking change.
There was a problem hiding this comment.
Oh interesting. Why are you padding your data outside of this blob in nitro?
But basically you can just use the new() constructor directly now, its doing this same unchecked loading (use from_raw_data if the data isnt already padded)
| /// Removes padding from the blob data if it is padded. | ||
| pub fn remove_padding(&mut self) -> Result<(), BlobError> { | ||
| if !self.is_padded { | ||
| Err(BlobError::NotPaddedError) | ||
| } else { | ||
| self.blob_data = | ||
| helpers::remove_empty_byte_from_padded_bytes_unchecked(&self.blob_data); | ||
| self.is_padded = false; | ||
| self.length_after_padding = 0; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Are we removing this function cause it isnt used anywhere?
There was a problem hiding this comment.
No removing because blobs are no longer stateful so we can't change them in place.
Still kept a function to_raw_data to get back the underlying data (unpad and give back the raw_data)
samlaf
force-pushed
the
style--issue14-cleanup-blob-struct
branch
from
17050bf to
dd40e9f
Compare
|
Rebased on top of master after having merged #22 |
|
did you merge without approval? I am ok with the change, but should we set the branch protection? |
Fixes #14
Contains some breaking api changes.
Also fixed the tests syntactically only, did not think very deeply about their semantics, so hopefully nothing breaks (doubt it though because this new API is much much simpler: blobs are no longer mutable and stateful).
@epociask tagging you since I think you are using this in nitro. Should be a pretty simple change to update to this new API.