-
Notifications
You must be signed in to change notification settings - Fork 1
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
Upgrade #19
Conversation
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.
Thank you for this! some organisation comments for your feedback!
@@ -23,9 +23,51 @@ pub enum SdjwtVerifierResultError { | |||
IdxRevoked(u64), | |||
} | |||
|
|||
impl std::fmt::Display for SdjwtVerifierResultError { |
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.
why impl and not just instead of using thiserror::Error
?
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.
Done 52c864a
packages/common/src/traits.rs
Outdated
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.
do we need this file? this is basically the expected ExecMsg
right?
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.
contracts/sdjwt-verifier/src/msg.rs
Outdated
|
||
// Execute messages | ||
#[cosmwasm_schema::cw_serde] | ||
pub enum ExecuteMsg { |
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.
these can be moved to the package so to act as "interface".
Please look at https://github.com/CosmWasm/cw-plus/tree/main/packages/cw20 and how the packages are used in the contract implementations.
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.
Good point, done 19c8202
contracts/sdjwt-verifier/Cargo.toml
Outdated
sd-jwt-rs = { workspace = true } | ||
jsonwebtoken = { workspace = true } | ||
serde_json = { workspace = true, default-features = false, features = ["alloc"]} | ||
serde-json-wasm = {workspace = true} |
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 saw this
CosmWasm/cosmwasm#2195
looks like we can replace remove it?
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.
Sure, f816656
|
||
use crate::types::{PendingRoute, VerificationRequirements}; | ||
|
||
pub const MAX_PRESENTATION_LENGTH: Item<usize> = Item::new("max_presentation_length"); |
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 would say some of these states can be moved to the package to be shared across verifier implementations.
For example, we have the typeVerfiablePresentation
as Binary
in the package because it will be required for any verifier, sd-jwt, anoncreds, zk, etc.
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.
Moved MAX_PRESENTATION_LENGTH
to package, let's see what we can reuse after we develop other verifiers.
52c864a
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.
great job!
Closes #16