Cardano: Add vote delegation to node api by RostarMarek · Pull Request #89 · BitBoxSwiss/bitbox-api-rs

RostarMarek · 2024-09-27T09:22:21Z

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.

refi93 · 2024-10-04T11:51:42Z

+  | {
+      voteDelegation: {
+        keypath: Keypath
+        type: number


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"

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,

benma · 2024-10-10T06:04:15Z

+  | {
+      voteDelegation: {
+        keypath: Keypath
+        type: number


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,

benma · 2024-10-10T06:05:32Z

+      voteDelegation: {
+        keypath: Keypath
+        type: number
+        drepCredhash: Uint8Array | undefined | null


why undefined or null? Seems like drepCredhash?: Uint8Array should be enough.

in scripts/build-protos.rs, you could rename this to drepCredHash (proper camelCase)

benma · 2024-10-10T06:08:22Z

+    );
 }

-export function Cardano({ bb02 } : Props) {


please keep the same style (spaces, indent level, ...)

benma

Please also add a CHANGELOG in CHANGELOG-npm.md under 0.7.0

If you rebase, the failing nightly CI item should disappear.

benma · 2024-10-28T11:14:54Z

+        (
+            "shiftcrypto.bitbox02.CardanoSignTransactionRequest.Certificate.VoteDelegation.CardanoDRepType.KEY_HASH",
+            "serde(rename = \"keyHash\")",
+        ),
+        (
+            "shiftcrypto.bitbox02.CardanoSignTransactionRequest.Certificate.VoteDelegation.CardanoDRepType.SCRIPT_HASH",
+            "serde(rename = \"scriptHash\")",
+        ),
+        (
+            "shiftcrypto.bitbox02.CardanoSignTransactionRequest.Certificate.VoteDelegation.CardanoDRepType.ALWAYS_ABSTAIN",
+            "serde(rename = \"alwaysAbstain\")",
+        ),
+        (
+            "shiftcrypto.bitbox02.CardanoSignTransactionRequest.Certificate.VoteDelegation.CardanoDRepType.ALWAYS_NO_CONFIDENCE",
+            "serde(rename = \"alwaysNoConfidence\")",
+        ),


These are not needed because camelCase is the default - the dRepType enum has this attribute already:

#[cfg_attr(feature = "wasm", serde(rename_all = "camelCase"))]

Could you remove these?

I think you missed make build-protos afterwards, as these tags are still in the generated protobuf file. Please run make build-protos again.

benma · 2024-10-28T11:18:26Z

+      voteDelegation: {
+        keypath: Keypath
+        type: CardanoDrepType
+        drepCredHash?: Uint8Array


you changed the name here but the deserialization still uses drepCredhash, so if you tried to use drepCredHash, it would not actually show the hash and error after confirmation.

benma · 2024-10-28T11:19:45Z

    ['zero-ttl', 'Transaction with TTL=0'],
    ['tokens', 'Transaction sending tokens'],
    ['delegate', 'Delegate staking to a pool'],
+    ['vote-delegation', 'Delegate vote to a dRep'],


Please add another one that delegates to a keyHash or scriptHash, or add a dropdown to the existing one to switch the mode so all of them can be tested in the sandbox. The hash ones don't work right now (see other comment).

benma

Also please add an entry to CHANGELOG-npm.md

benma · 2024-10-31T08:28:37Z

In the meantime we released v0.7.0 to npmjs. Please rebase and change NPM_VERSION to 0.8.0. Thanks.

benma · 2024-11-14T13:43:56Z


 ## Unreleased

+## 0.8.0


please also change NPM_VERSION - https://github.com/BitBoxSwiss/bitbox-api-rs/blob/master/NPM_VERSION

benma · 2024-11-14T13:45:17Z

+        (
+            "shiftcrypto.bitbox02.CardanoSignTransactionRequest.Certificate.VoteDelegation.CardanoDRepType.KEY_HASH",
+            "serde(rename = \"keyHash\")",
+        ),
+        (
+            "shiftcrypto.bitbox02.CardanoSignTransactionRequest.Certificate.VoteDelegation.CardanoDRepType.SCRIPT_HASH",
+            "serde(rename = \"scriptHash\")",
+        ),
+        (
+            "shiftcrypto.bitbox02.CardanoSignTransactionRequest.Certificate.VoteDelegation.CardanoDRepType.ALWAYS_ABSTAIN",
+            "serde(rename = \"alwaysAbstain\")",
+        ),
+        (
+            "shiftcrypto.bitbox02.CardanoSignTransactionRequest.Certificate.VoteDelegation.CardanoDRepType.ALWAYS_NO_CONFIDENCE",
+            "serde(rename = \"alwaysNoConfidence\")",
+        ),


I think you missed make build-protos afterwards, as these tags are still in the generated protobuf file. Please run make build-protos again.

Added vote delegation to node api and wasm generation. Signed-off-by: RostarMarek <rostarmarek@gmail.com>

benma

Thanks 🎉

RostarMarek force-pushed the rostarmarek/cardano/npm-vote-delegation branch from 22aaf65 to abcd7f4 Compare September 27, 2024 11:08

RostarMarek marked this pull request as ready for review September 27, 2024 12:12

refi93 reviewed Oct 4, 2024

View reviewed changes

benma requested changes Oct 10, 2024

View reviewed changes

benma reviewed Oct 10, 2024

View reviewed changes

RostarMarek force-pushed the rostarmarek/cardano/npm-vote-delegation branch 3 times, most recently from 00d5d6a to eb4173a Compare October 11, 2024 11:20

benma requested changes Oct 28, 2024

View reviewed changes

RostarMarek force-pushed the rostarmarek/cardano/npm-vote-delegation branch 2 times, most recently from f211ed6 to 18b8237 Compare November 12, 2024 00:33

RostarMarek requested a review from benma November 12, 2024 10:42

benma requested changes Nov 14, 2024

View reviewed changes

Cardano: Add vote delegation to node api

afe26fd

Added vote delegation to node api and wasm generation. Signed-off-by: RostarMarek <rostarmarek@gmail.com>

RostarMarek force-pushed the rostarmarek/cardano/npm-vote-delegation branch from 18b8237 to afe26fd Compare November 18, 2024 12:05

benma approved these changes Nov 18, 2024

View reviewed changes

benma merged commit 49e5ab8 into BitBoxSwiss:master Nov 18, 2024

Conversation

RostarMarek commented Sep 27, 2024

Uh oh!

refi93 Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

benma Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

benma Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

benma Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

benma Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

benma left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

benma Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

benma left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma commented Oct 31, 2024

Uh oh!

benma Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benma left a comment •

edited

Loading

benma Nov 14, 2024 •

edited

Loading

benma Oct 28, 2024 •

edited

Loading

benma left a comment •

edited

Loading

benma Nov 14, 2024 •

edited

Loading

benma Nov 14, 2024 •

edited

Loading