Skip to content

Metadata - #1

Merged
leonbotros merged 12 commits into
masterfrom
file-version-metadata
Apr 23, 2021
Merged

Metadata#1
leonbotros merged 12 commits into
masterfrom
file-version-metadata

Conversation

@rowanG077

@rowanG077 rowanG077 commented Sep 21, 2020

Copy link
Copy Markdown
Contributor

Moved metadata to json.

TODOs:

  • Make old test pass
  • Make new tests
  • Rewrite packages depending on irmaseal-core to use new API.

@rowanG077
rowanG077 requested a review from ivard September 21, 2020 17:35
@rowanG077
rowanG077 force-pushed the file-version-metadata branch from d2d8fec to 6bb29b5 Compare September 22, 2020 09:52
Comment thread irmaseal-core/src/meta.rs Outdated
Comment thread irmaseal-core/src/meta.rs Outdated
Comment thread irmaseal-core/src/meta.rs Outdated
@rowanG077 rowanG077 changed the title WIP: Metadata Metadata Oct 20, 2020
@ivard

ivard commented Oct 28, 2020

Copy link
Copy Markdown

The unit tests work fine, only when I try to compile it as part of irmaseal-cli, I get compile errors. Since irmaseal-cli is included in this repository, this compile error can be fixed within this PR. In Cargo.tom you can set irmaseal-core to { path = "../irmaseal-core" }. Then you'll see the error.

@Wassasin

Wassasin commented Nov 2, 2020

Copy link
Copy Markdown

Do not make irmaseal-core std. When you want to run this in an embedded context (for a secure element) or webassembly having this crate be no-std is essential.

Furthermore I think that having json in a file header is wasteful. There are currently no compact, non-alloc, no-std, serde-compatible serialization crates that have a nice format specification. If you drop the format specification requirement I recommend the postcard crate. I have exhaustively looked into this for other (embedded) projects. If it is a requirement, you could consider paying the postcard author to finish the specification document.

@rowanG077
rowanG077 requested a review from Wassasin November 3, 2020 13:35
@rowanG077

Copy link
Copy Markdown
Contributor Author

@Wassasin I moved to postcard and removed serde_json. Seems to work fine with the unit tests. Still todo is to move the CLI tool and pkg has not been migrated yet.

@leonbotros
leonbotros force-pushed the file-version-metadata branch from 5273963 to 599a8a6 Compare November 18, 2020 13:11

@leonbotros leonbotros left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall, its looks good. I left some questions though.

Comment thread irmaseal-cli/src/decrypt.rs Outdated
Comment thread irmaseal-cli/src/encrypt.rs Outdated
Comment thread irmaseal-core/src/identity.rs Outdated
#[test]
fn eq_write_read() {
let mut buf = IdentityBuf::new();
let mut buf: [u8; 1024] = [0; 1024];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can't use the IdentityBuf type alias here?

Comment thread irmaseal-core/src/identity.rs Outdated
const IDENTITY_UNSET: u8 = 0xFF;

// Must be at least 8+1+255+1+254 = 519
// Must be at least 8+255+1+254 = 518

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why this change? We would still need 1 byte to encode the length of both ArrayStrings?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not that it matters...

Comment thread irmaseal-core/src/stream/mod.rs
Comment thread irmaseal-core/src/stream/sealer.rs
Comment thread irmaseal-pkg/Cargo.toml
@rowanG077

Copy link
Copy Markdown
Contributor Author

@Wassasin Could you quickly look over the changes if everything is Oké with you?

Comment thread irmaseal-cli/src/cli.yml
help: Sets the input file to use
required: true
index: 1
- OUTPUT:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it really practical to remove this? If I have a file that is named exactly the same as a file within the tar, what happens then? Is it overwritten then or do I get an error?

@Iso5786
Iso5786 force-pushed the file-version-metadata branch from 251d9be to 6ca580a Compare April 15, 2021 09:44
@Iso5786
Iso5786 force-pushed the file-version-metadata branch from 6ca580a to 20740f3 Compare April 15, 2021 11:38
@leonbotros
leonbotros removed the request for review from Wassasin April 23, 2021 07:48
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.

5 participants