Adds payload support in PSKT #703
Conversation
wallet/pskt/src/pskt.rs
Outdated
| pub fn payload(mut self, payload: Vec<u8>) -> Self { | ||
| self.inner_pskt.global.payload = Some(payload); | ||
| self | ||
| } |
There was a problem hiding this comment.
Could be wise to keep the underlying signature (Option<Vec<u8>>), some might want to reset payload by setting a None value to it.
|
Hello, @danwt great work.
If overwriting is required - it can be solved via another pr/issue or it should require separate field with vector of payloads so the finalized can combine them properly, or some other deterministic approach. The invariant is left+right==right+left |
| /// Unknown key-value pairs for this output. | ||
| #[serde(flatten)] | ||
| pub unknowns: BTreeMap<String, serde_value::Value>, | ||
| pub payload: Option<Vec<u8>>, |
There was a problem hiding this comment.
Add attribute #[serde(with = "kaspa_utils::serde_bytes_optional")] to serialize it as hex in case of human readable serializer, as well as it has consistency with redeem script serialization
There was a problem hiding this comment.
Thanks very much for review I'll address soon
- Add Version::One variant to PSKT Version enum for explicit payload support - Add serde attribute for hex serialization of payload field - Implement payload combination rules in Global::Add: - Both None -> None - One has payload -> use that payload - Same payload -> use that payload - Different payloads -> return PayloadMismatch error - Add PayloadMismatch error variant to CombineError enum - Auto-upgrade PSKT version to One when setting payload - Maintain backward compatibility with Version::Zero (no payload) These changes address all feedback from the PR review while maintaining compatibility and proper error handling.
- Changed payload() method to return Result<Self, Error> instead of Self - Payload can only be set when PSKT version is One or higher (no auto-upgrade) - Returns PayloadRequiresVersion1 error if attempting to set payload on Version::Zero - unsigned_tx() now only includes payload when version >= One - Version combining already fails if versions don't match (no changes needed) This ensures strict compatibility: old clients (Version::Zero) cannot have payloads, and combining PSKTs with different versions will fail as expected.
Address reviewer feedback for PSKT payload support
wallet/pskt/src/global.rs
Outdated
| // - One has payload -> use that payload | ||
| // - Both have same payload -> use that payload | ||
| // - Different payloads -> error | ||
| self.payload = match (self.payload.clone(), rhs.payload.clone()) { |
There was a problem hiding this comment.
You can use .take instead of clone here
There was a problem hiding this comment.
It also should return error in case of version<1
Could you also add a set_version method to pskt?
There was a problem hiding this comment.
PR looks legit. What else is pending here @biryukovmaxim @danwt?
There was a problem hiding this comment.
|
LGTM, |
|
Looks good. @danwt Please merge or rebase with latest master |
4 participants
Closes #700