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

Cardano Conway certificates #28

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

jaskp
Copy link

@jaskp jaskp commented Dec 3, 2023

Description

Related Issue

Resolve

Screenshots:

@jaskp jaskp self-assigned this Dec 3, 2023
@jaskp jaskp force-pushed the feat/cardano-conway-certificates branch from e6c0059 to 3fcb6f1 Compare December 5, 2023 13:57
@jaskp jaskp marked this pull request as ready for review December 5, 2023 13:57
Copy link
Collaborator

@davidmisiak davidmisiak left a comment

Choose a reason for hiding this comment

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

LGTM, let's add the remaining stuff when fw is done.

@jaskp jaskp force-pushed the feat/cardano-conway-certificates branch 2 times, most recently from c6074b9 to 7c9fe02 Compare December 13, 2023 16:34
Copy link
Collaborator

@davidmisiak davidmisiak left a comment

Choose a reason for hiding this comment

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

Except for the naming that is being discussed, I haven't spotted any issue.

Can you please update the docs here though? (Sorry I haven't realized that earlier.)

Copy link
Collaborator

@janmazak janmazak left a comment

Choose a reason for hiding this comment

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

I don't understand Trezor code in detail, so some of the comments might be meaningless.

I did not test anything.

URL and DNS length was increased to 128, it is not included in this PR --- will it be in another?

@jaskp
Copy link
Author

jaskp commented Jan 13, 2024

URL and DNS length was increased to 128, it is not included in this PR --- will it be in another?

The length isn't being validated in suite, just firmware

@jaskp jaskp force-pushed the feat/cardano-conway-certificates branch from fba08ee to 2766de5 Compare January 16, 2024 21:32
process.chdir(outputDirectory);

// Run npm pack
const result = execSync(`npm pack ${modulePath}`, { encoding: 'utf8' });

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
@jaskp jaskp force-pushed the feat/cardano-conway-certificates branch 4 times, most recently from eb99401 to 28c615e Compare January 17, 2024 09:26
Copy link
Collaborator

@davidmisiak davidmisiak left a comment

Choose a reason for hiding this comment

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

I ckecked the validation changes, looks good 👍 Please don't forget to re-run the integration tests before opening the upstream PR.

@davidmisiak davidmisiak force-pushed the feat/cardano-conway-certificates branch from 28c615e to 0d89e41 Compare May 24, 2024 19:34
@davidmisiak davidmisiak force-pushed the feat/cardano-conway-certificates branch from 0d89e41 to fd92a5c Compare May 31, 2024 09:54
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