Add Vote Delegation
#45
Conversation
This reverts commit 5fb4c0e.
|
@coderabbitai full review |
There was a problem hiding this comment.
Pull Request Overview
This PR adds Vote Delegation support across multiple modules by updating certificate types, serialization/deserialization logic, and related API functions.
- Updated tx_builder to properly extract redeemer bytes with tuple destructuring.
- Introduced new serialization and deserialization routines for VoteDelegation and associated types such as DRep.
- Extended CertificateKind and CertificateEnum with new variants and API methods for VoteDelegation.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rust/src/tx_builder.rs | Adjusted iteration mapping and added handling for VoteDelegation certificates. |
| rust/src/serialization.rs | Added new helper functions for index checks and VoteDelegation serialization/deserialization; updated CertificateEnum serialization to include VoteDelegation. |
| rust/src/lib.rs | Extended certificate enums with additional variants and new VoteDelegation API methods. |
| rust/src/error.rs | Added FixedValuesMismatch variant to DeserializeFailure. |
| rust/src/address.rs | Introduced has_script_hash method for StakeCredential. |
| rust/Cargo.toml | Updated dependency revision for uplc. |
Comments suppressed due to low confidence (2)
rust/src/serialization.rs:1984
- Avoid panicking on unhandled certificate variants in the serialization logic. Consider returning a Result with an appropriate error instead for better API resilience.
CertificateEnum::VoteDelegation(x) => x.serialize(serializer),
rust/src/lib.rs:1240
- Using panic for undefined certificate kinds may lead to unexpected shutdowns at runtime. It is recommended to handle this case gracefully, possibly by returning a default value or error.
// For all other variants, return a default or placeholder value, or handle as needed.
|
@CodeRabbit full review |
|
@coderabbitai help |
CodeRabbit Commands (Invoked using PR comments)
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes introduce new certificate types and related structures for vote delegation and decentralized representatives (DReps) in the Rust codebase. Serialization, deserialization, and transaction witness logic are extended to handle these new variants. Additional utility functions and unit tests are added to support and validate the new functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Certificate
participant VoteDelegation
participant DRep
User->>Certificate: new_vote_delegation(VoteDelegation)
Certificate->>VoteDelegation: Access stake_credential, drep
VoteDelegation->>DRep: kind(), to_key_hash(), to_script_hash()
Certificate->>Certificate: kind() (returns VoteDelegation)
sequenceDiagram
participant Serializer
participant VoteDelegation
participant DRep
Serializer->>VoteDelegation: serialize()
VoteDelegation->>Serializer: serialize stake_credential
VoteDelegation->>DRep: serialize()
DRep->>Serializer: serialize DRepEnum variant
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
rust/src/serialization.rs (2)
2015-2102: Add VoteDelegation deserialization to CertificateEnumThe
CertificateEnumdeserialization is missing theVoteDelegationvariant. This will cause deserialization failures for vote delegation certificates.Add the following match arm after line 2096:
}; + match (|raw: &mut Deserializer<_>| -> Result<_, DeserializeError> { + Ok(VoteDelegation::deserialize_as_embedded_group(raw, len)?) + })(raw) + { + Ok(variant) => return Ok(CertificateEnum::VoteDelegation(variant)), + Err(_) => raw + .as_mut_ref() + .seek(SeekFrom::Start(initial_position)) + .unwrap(), + }; Err(DeserializeError::new( "CertificateEnum", DeserializeFailure::NoVariantMatched.into(),
5124-5739: Missing tests for VoteDelegation and DRepWe’ve confirmed there are no
#[test]functions covering the new VoteDelegation or DRep types. To ensure reliability, please add unit tests that cover:• VoteDelegation
– Round-trip CBOR serialization/deserialization viaVoteDelegation::to_bytes()/from_bytes()
– Integration inCertificateEnum::VoteDelegationround trips
– Edge cases (e.g. votes for different credentials, invalid inputs)• DRep
– Round-trip CBOR serialization/deserialization viaDRep::to_bytes()/from_bytes()
– All enum variants (Key,Script,Abstain,NoConfidence)
– Integration in any surrounding structures (e.g. transaction builder)Would you like example test stubs to get started?
rust/src/lib.rs (1)
1187-1294: Add accessor methods for new certificate types.While
new_vote_delegationconstructor is added, theCertificatestruct is missing accessor methods (likeas_vote_delegation) for the new certificate types. This inconsistency with existing patterns (e.g.,as_stake_registration,as_pool_registration) should be addressed.Add accessor methods following the existing pattern:
pub fn as_vote_delegation(&self) -> Option<VoteDelegation> { match &self.0 { CertificateEnum::VoteDelegation(x) => Some(x.clone()), _ => None, } }
🧹 Nitpick comments (3)
rust/src/address.rs (1)
141-146: LGTM! Consider consistency with existing patterns.The implementation is correct and follows the established pattern of using pattern matching on the internal enum. The method provides a convenient boolean check for script hash presence.
Note: This method could also be implemented as
self.kind() == StakeCredKind::Scriptfor consistency with the existingkind()method, but the current approach is equally valid and potentially more readable.rust/src/serialization.rs (2)
6-6: Remove unnecessary importThe
use core::panic;import is unnecessary aspanic!is already available in the standard prelude.-use core::panic;
1714-1727: Consider optimizing error message allocationThe
format!macro allocates a String on every error, which could impact performance in hot paths. Consider using a static error message or implementing a custom error type.- None => Err(cbor_event::Error::CustomError(format!( - "unknown index of {}", - name - ))), + None => Err(cbor_event::Error::CustomError( + "unknown index for serialization".to_string() + )),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rust/Cargo.toml(1 hunks)rust/src/address.rs(1 hunks)rust/src/error.rs(2 hunks)rust/src/lib.rs(5 hunks)rust/src/serialization.rs(4 hunks)rust/src/tx_builder.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rust/src/tx_builder.rs (3)
rust/src/address.rs (2)
from_bytes(236-240)from_keyhash(112-114)rust/src/utils.rs (1)
from_bytes(28-31)rust/src/lib.rs (23)
new_always_abstain(1108-1110)new_key_hash(1100-1102)stake_credential(683-685)stake_credential(706-708)stake_credential(730-732)stake_credential(1153-1155)new(96-101)new(147-157)new(175-177)new(208-210)new(276-278)new(518-529)new(582-587)new(661-668)new(687-691)new(710-714)new(738-743)new(756-758)new(791-793)new(864-886)new(905-909)new(933-938)new_vote_delegation(1187-1189)
🔇 Additional comments (9)
rust/Cargo.toml (1)
38-38: Manual verification required: inspect uplc v1.1.13 diffOur attempt to list commits between
3d77b5c378ce404cddd9a1f111906d72fd46fc83and900bf91216deff19be9b3582a9fccd03be0d485creturned no output—likely due to a shallow clone. Please manually confirm that moving to tag v1.1.13:
- Only includes the intended vote delegation changes
- Does not introduce other breaking changes
Location to check:
- rust/Cargo.toml, line 38:
uplc = { git = "https://github.com/aiken-lang/aiken.git", rev = "900bf91216deff19be9b3582a9fccd03be0d485c", default-features = false }rust/src/error.rs (2)
37-40: LGTM! Enhanced error reporting capability.The new
FixedValuesMismatchvariant properly extends the error handling to support multiple expected values, which is useful for cases where several valid options exist.
111-113: LGTM! Consistent error message formatting.The Display implementation correctly handles the new variant by formatting the expected values using debug representation, which is appropriate for showing a vector of keys.
rust/src/tx_builder.rs (4)
104-108: LGTM! Vote delegation certificate handling looks correct.The implementation properly extracts the stake credential from the vote delegation certificate and adds the key hash to the witness set when present. This follows the established pattern for other certificate types and correctly handles the optional nature of key hashes.
2052-2057: LGTM! Basic DRep serialization test is appropriate.The test verifies that
DRep::new_always_abstain()serializes to the expected CBOR hex value. The test is simple and focused on the core functionality.
2059-2076: LGTM! Comprehensive vote delegation test with good coverage.The test thoroughly validates the vote delegation certificate creation and serialization process:
- Creates DRep with key hash
- Creates stake credential from key hash
- Constructs VoteDelegation certificate
- Verifies final CBOR encoding
The test covers the end-to-end flow and uses realistic hex values for validation.
71-71: I need to inspect howredeemers_bytesis produced and the surrounding context to confirm the tuple structure. Let me pull in the relevant section ofrust/src/tx_builder.rs.rust/src/lib.rs (2)
1076-1138: Well-structured DRep implementation.The
DReptype is properly implemented with all necessary constructors, conversion methods, and a clear enum structure for different DRep variants.
1168-1170: has_script_hash is already implemented on StakeCredentialStakeCredential defines
has_script_hashinrust/src/address.rs(at line 141), so the call toself.stake_credential.has_script_hash()is valid. No changes required.Likely an incorrect or invalid review comment.
|
@coderabbitai thanks for review, I pushed commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ljttl3q04t I'll help you check the commit to verify that the fixes address the review concerns. Let me examine the changes in that commit. 🏁 Script executed: Length of output: 268870 @ljttl3q04t Got it — I’ll run an up-to-date diff on the key files and re-check the earlier review points now. 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rust/src/serialization.rs (1)
1770-1778: Consider clarifying the error message for DRepEnum array length validation.The error message specifies expected length as
2, but the code actually accepts both 1 and 2 element arrays depending on the variant. Consider making this more descriptive:return Err(DeserializeFailure::CBOR(cbor_event::Error::WrongLen( - 2, + 1, // or update message to indicate "1 or 2" len, - "[id, hash] or [id] (for abstain and no confidence)", + "[id] for abstain/no confidence or [id, hash] for key/script variants", ))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rust/src/lib.rs(5 hunks)rust/src/serialization.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/lib.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ljttl3q04t
PR: minswap/cardano-serialization-lib#45
File: rust/src/lib.rs:1050-1074
Timestamp: 2025-07-01T03:53:00.461Z
Learning: In the minswap/cardano-serialization-lib repository, dummy/placeholder structs for certificate types (like CommitteeHotAuth, CommitteeColdResign, etc.) are acceptable when they are not currently used and are intended as placeholders for future implementation.
Learnt from: ljttl3q04t
PR: minswap/cardano-serialization-lib#45
File: rust/src/serialization.rs:1983-1985
Timestamp: 2025-07-01T03:57:39.025Z
Learning: In the minswap/cardano-serialization-lib repository, using panic! for unknown/unused CertificateEnum variants in serialization is acceptable as a simple solution when those certificate types are never used in the codebase.
rust/src/serialization.rs (2)
undefined
<retrieved_learning>
Learnt from: ljttl3q04t
PR: #45
File: rust/src/serialization.rs:1983-1985
Timestamp: 2025-07-01T03:57:39.025Z
Learning: In the minswap/cardano-serialization-lib repository, using panic! for unknown/unused CertificateEnum variants in serialization is acceptable as a simple solution when those certificate types are never used in the codebase.
</retrieved_learning>
<retrieved_learning>
Learnt from: ljttl3q04t
PR: #45
File: rust/src/lib.rs:1050-1074
Timestamp: 2025-07-01T03:53:00.461Z
Learning: In the minswap/cardano-serialization-lib repository, dummy/placeholder structs for certificate types (like CommitteeHotAuth, CommitteeColdResign, etc.) are acceptable when they are not currently used and are intended as placeholders for future implementation.
</retrieved_learning>
🧬 Code Graph Analysis (1)
rust/src/serialization.rs (1)
rust/src/error.rs (2)
from(144-146)from(150-155)
🔇 Additional comments (5)
rust/src/serialization.rs (5)
1684-1725: LGTM: Well-designed utility functions for serialization validation.These utility functions provide excellent error handling and consistent interfaces for index and serialization validation. The error propagation and conversion logic is correct, and the functions will help ensure consistency across different certificate types.
1727-1811: LGTM: Comprehensive DRep serialization implementation.The DRep serialization correctly handles all four variants with appropriate CBOR array structures:
- KeyHash/ScriptHash variants use 2-element arrays with index + data
- AlwaysAbstain/AlwaysNoConfidence use 1-element arrays with just the index
- Deserialization properly validates array lengths and variant indices
- Error handling uses the appropriate
FixedValuesMismatchfor multiple expected values
1813-1826: LGTM: VoteDelegation index consistency resolved.The VoteDelegation implementation now consistently uses index 9 across all serialization methods:
serialize()uses index 9 (line 1820)serialize_as_embedded_group()uses index 9 (line 1915)deserialize_as_embedded_group()expects index 9 (line 1954)This addresses the index mismatch issue mentioned in previous reviews.
Also applies to: 1910-1920, 1948-1967
1892-1908: LGTM: Useful length validation utility.The
check_lenfunction provides good validation for CBOR array lengths with descriptive error messages. The implementation correctly handles both definite and indefinite length arrays.
1982-1982: LGTM: Proper integration with existing certificate serialization.The VoteDelegation variant is correctly added to the CertificateEnum serialization dispatch. The placement and pattern matches the existing code structure.
|
@coderabbitai hey, if you have no concern then approve this PR please |
|
Tip For best results, initiate chat on the files or code changes. @ljttl3q04t I appreciate your confidence in my review! However, I need to clarify that as an AI assistant, I cannot actually approve PRs on GitHub. Only human reviewers with the appropriate repository permissions can approve pull requests. What I can do is:
Based on the commit you pushed ( To get your PR approved, you should:
Would you like me to do a final review check to ensure everything looks good before the maintainers take a look? |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation