feat: track cdk version in keysets #1556
Conversation
a1denvalu3
force-pushed
the
feat/keyset-cdk-versioning
branch
from
2e2c75b to
157a708
Compare
thesimplekid
left a comment
thesimplekid
left a comment
There was a problem hiding this comment.
Do you think we need to write more then just the version number but also that its cdk? I am think about a case we we migration a nutshell mint, we would want to know what ones are nutshell vs cdk and with just the version we could not tell this?
|
@thesimplekid how do you suggest doing this? |
| @@ -368,3 +435,112 @@ pub struct FeeReserve { | |||
| /// Percentage expected fee | |||
| pub percent_fee_reserve: f32, | |||
| } | |||
|
|
|||
| /// CDK Version | |||
| #[derive(Debug, Clone, Hash, PartialEq, Eq)] | |||
| pub struct CdkVersion { | |||
| /// Implementation name (e.g., "cdk", "nutshell") | |||
| pub implementation: String, | |||
There was a problem hiding this comment.
Can we move these above the test mod? The test mod should be last in the file.
There was a problem hiding this comment.
Can we include these tests in the test mod of crates/cdk-common/src/common.rs where the CdkVersion is defined, we shouldn't need a separate mod here.
| Keys keys = 5; | ||
| optional uint64 final_expiry = 6; | ||
| uint64 version = 7; | ||
| optional string cdk_version = 8; |
There was a problem hiding this comment.
I think this should just be version? In theory this should be general for all signatory though the spec is not merged. @lescuer97
There was a problem hiding this comment.
just "version" would be confusing.
There was a problem hiding this comment.
Yeah true, software version? I'm not sure just something not cdk specefic I think.
There was a problem hiding this comment.
how version is used right now is to specify the order of the keysets.
mint_version is probably fine.
There was a problem hiding this comment.
issuer would be a good name here
3 participants
cdk_versioncolumn tokeysettable (Postgres/SQLite).env!("CARGO_PKG_VERSION")on keyset generation.closes: #1552