Skip to content
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(rust/vote-tx-v2): Public vote tx v2 CBOR decoding/encoding implementation #86

Merged
merged 30 commits into from
Dec 2, 2024

Conversation

Mr-Leshiy
Copy link
Contributor

Description

  • Updated cddl specs for the v2 vote transactions, made voter-data type generic, the same as choice, proof, and prop-id.
  • Made GeneralizedTx structure generic according to the original cddl spec.
  • Refactored GeneralizedTx code structured it across different Rust modules.
  • Added GeneralizedTxBuilder structure.
  • Added missing validation during the decoding GeneralizedTx.
  • Added new PublicTx structure with the corresponding Choice, Proof and PropId types definitions according to the cddl spec.
  • Implemented CBOR encoding/decoding functionality for PublicTx and all underlying types.

Related Issue(s)

#84

@Mr-Leshiy Mr-Leshiy self-assigned this Nov 19, 2024
@Mr-Leshiy Mr-Leshiy added the review me PR is ready for review label Nov 19, 2024
@Mr-Leshiy Mr-Leshiy changed the title feat(vote-tx-v2): Public vote tx v2 CBOR decoding/encoding implementation feat(rust/vote-tx-v2): Public vote tx v2 CBOR decoding/encoding implementation Nov 19, 2024
Copy link
Contributor

github-actions bot commented Nov 19, 2024

Test Report | ${\color{lightgreen}Pass: 228/228}$ | ${\color{red}Fail: 0/228}$ |

@Mr-Leshiy Mr-Leshiy requested a review from cong-or November 19, 2024 10:35
@Mr-Leshiy Mr-Leshiy marked this pull request as draft November 19, 2024 10:42
@Mr-Leshiy Mr-Leshiy added do not merge yet PR is not ready to merge yet and removed review me PR is ready for review labels Nov 19, 2024
@Mr-Leshiy Mr-Leshiy marked this pull request as ready for review November 19, 2024 11:02
@Mr-Leshiy Mr-Leshiy added review me PR is ready for review and removed do not merge yet PR is not ready to merge yet labels Nov 19, 2024
@minikin minikin self-requested a review November 25, 2024 12:01
Copy link
Collaborator

@minikin minikin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only suggestion I have. To consider creating a module for vote-related types:

pub mod vote {
    pub trait CborSerializable: for<'a> Cbor<'a> {}
    impl<T: for<'a> Cbor<'a>> CborSerializable for T {}
    
    pub type VoteChoice = Box<dyn CborSerializable>;
    pub type VoteProof = Box<dyn CborSerializable>;
    pub type ProposalId = Box<dyn CborSerializable>;
    ...
    ...
    ...
}

use crate::{encoded_cbor::EncodedCbor, Cbor};

/// `Vote` array struct length
const VOTE_LEN: u64 = 3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is the 3 defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is defined from the cddl spec

vote<choice-t, proof-t, prop-id-t>  = [
    choices<choice-t>,
    proof<proof-t>,
    prop-id<prop-id-t>,
]

so basically, vote type here a cbor encoded array with the 3 field in it.
Also I've discussed with @stevenj that we should allow only definite sized arrays and maps in such cases (because cbor also allows to have indefinite size arrays and maps and cddl does not specify it). I will put a note into our spec about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cong-or so probably we will also need to revisit ledger encoding implementation to double check that it encodes as a finite cbor arrays and cbor maps for the types, but its not urgent and for now its totally ok if its not strictly verifies it.

@Mr-Leshiy Mr-Leshiy requested a review from cong-or December 2, 2024 09:40
cong-or
cong-or previously approved these changes Dec 2, 2024
Copy link
Contributor

@cong-or cong-or left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Mr-Leshiy Mr-Leshiy requested review from cong-or and minikin December 2, 2024 10:38
@Mr-Leshiy Mr-Leshiy enabled auto-merge (squash) December 2, 2024 10:59
@minikin minikin disabled auto-merge December 2, 2024 11:28
@minikin minikin merged commit e3b9407 into main Dec 2, 2024
24 checks passed
@minikin minikin deleted the feat/public-private-vote-tx branch December 2, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review me PR is ready for review
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants