-
Notifications
You must be signed in to change notification settings - Fork 67
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(applying): add missing validation checks, prot params feature and documentation #325
Conversation
pallas-applying/Cargo.toml
Outdated
@@ -13,8 +13,14 @@ authors = ["Maico Leberle <[email protected]>"] | |||
doctest = false | |||
|
|||
[dependencies] | |||
cryptoxide = "0.1" | |||
cbor_event = "^2.1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have a crate for dealing with cbor serialization, it's called minicbor
. You can access it via pallas::codec::minicbor
. Any particular reason we need cbor_event
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just trying to copy the implementation in rust-byron-cardano. But nevermind, I just added a commit changing it minicbor
instead.
pallas-applying/src/byron.rs
Outdated
se.serialize(&tx_hash.as_ref()) | ||
.map_err(|_| ValidationError::UnableToProcessWitnesses)?; | ||
let data_to_verify: Vec<u8> = se.finalize(); | ||
if !ed25519::verify(&data_to_verify, &pub_key.as_slice()[0..32], signature) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have a function in pallas::crypto::keys
to verify ed25519. We should use that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added a commit fixing this.
pallas-applying/Cargo.toml
Outdated
@@ -13,8 +13,14 @@ authors = ["Maico Leberle <[email protected]>"] | |||
doctest = false | |||
|
|||
[dependencies] | |||
cryptoxide = "0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crypto algos should be made available through pallas::crypto
, which already depends on cryptoxide
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added a commit fixing this.
pallas-applying/tests/byron.rs
Outdated
const MAINNET_TX_HEX_CBOR: &str = | ||
"82839f8200d8185824825820da832fb5ef57df5b91817e9a7448d26e92552afb34f8ee5adb491b24bbe990d50e\ | ||
ff9f8282d818584283581cdac5d9464c2140aeb0e3b6d69f0657e61f51e0c259fe19681ed268e8a101581e581c2\ | ||
b5a44277e3543c08eae5d9d9d1146f43ba009fea6e285334f2549be001ae69c4d201b0000000172a84e408282d8\ | ||
18584283581c2b8e5e0cb6495ec275872d1340b0581613b04a49a3c6f2f760ecaf95a101581e581cca3e553c9c6\ | ||
3c5b66689e943ce7dad7d560ae84d7c2eaf21611c024c001ad27c159a1b00000003355d95efffa0818200d81858\ | ||
85825840888cdf85991d85f2023423ba4c80d41570ebf1fc878c9f5731df1d20c64aecf3e8aa2bbafc9beba8ef3\ | ||
3acb4d7e199b445229085718fba83b7f86ab6a3bcf782584063e34cf5fa6d8c0288630437fa5e151d93907e826e\ | ||
66ba273145e3ee712930b6f446ff81cb91d7f0cb4ceccd0466ba9ab14448d7eab9fc480a122324bd80170e"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required for now, but we should load test data from the test_data
folder where we already hold test vectors for different crates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this commit I moved the CBOR to test_data
cf3067f
to
b286707
Compare
No description provided.