-
Notifications
You must be signed in to change notification settings - Fork 9
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: Add vote delegation to node api #89
base: master
Are you sure you want to change the base?
Cardano: Add vote delegation to node api #89
Conversation
22aaf65
to
abcd7f4
Compare
src/wasm/types.rs
Outdated
| { | ||
voteDelegation: { | ||
keypath: Keypath | ||
type: number |
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.
number
is a bit too generic, wouldn't it be possible to narrow the values down with some enum? That would potentially allow for making this a union of different object shapes, making it clear when is drep hash required and when it isn't
Moreover, if possible, instead of null, I'd consider making the drepCredHash
optional, at least from a typescript perspective it would feel more "natural"
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 agree, this type should be something like
CardanoDRepType = 'keyHash' | 'scriptHash' | 'alwaysAbstain' | 'alwaysNoConfidence'
if it doesn't work out of the box like this, check other enums (e.g. in btc.proto/btc.rs) and scripts/build-protos.rs for reference,
src/wasm/types.rs
Outdated
| { | ||
voteDelegation: { | ||
keypath: Keypath | ||
type: number |
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 agree, this type should be something like
CardanoDRepType = 'keyHash' | 'scriptHash' | 'alwaysAbstain' | 'alwaysNoConfidence'
if it doesn't work out of the box like this, check other enums (e.g. in btc.proto/btc.rs) and scripts/build-protos.rs for reference,
src/wasm/types.rs
Outdated
voteDelegation: { | ||
keypath: Keypath | ||
type: number | ||
drepCredhash: Uint8Array | undefined | null |
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 undefined or null? Seems like drepCredhash?: Uint8Array
should be enough.
in scripts/build-protos.rs, you could rename this to drepCredHash
(proper camelCase)
sandbox/src/Cardano.tsx
Outdated
} | ||
|
||
export function Cardano({ bb02 } : Props) { |
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.
please keep the same style (spaces, indent level, ...)
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.
Please also add a CHANGELOG in CHANGELOG-npm.md under 0.7.0
If you rebase, the failing nightly CI item should disappear.
2df5a0d
to
00d5d6a
Compare
Added vote delegation to node api and wasm generation. Signed-off-by: RostarMarek <[email protected]>
00d5d6a
to
eb4173a
Compare
Added vote delegation to node api and wasm generation.
I still have the issue where I can't build that I mentioned here so I wasn't able to load firmware to my dev device to properly test it out.