Zsa Swap Utils by alexeykoren · Pull Request #141 · QED-it/orchard

alexeykoren · 2025-02-19T09:18:45Z

This PR contains a set of additions/changes that allow ZSA Swaps to be implemented in librustzcash:

Adds Swap BundleType and Flag
Adds AuthorizedWithProof trait that defines proof() method for both Authorized and ActionGroupAuthorized
Adds utility methods e.g. from_parts() for SwapBundle, is_empty() in Builder
Changes visibility of some methods e.g. dummy() for testing in librustzcash
Removes ActionGroup structure

PaulLaux

added comments.
AuthorizedWithProof is critical

PaulLaux · 2025-02-19T17:07:03Z

src/bundle.rs

 const FLAG_OUTPUTS_ENABLED: u8 = 0b0000_0010;
 const FLAG_ZSA_ENABLED: u8 = 0b0000_0100;
-const FLAGS_EXPECTED_UNSET: u8 = !(FLAG_SPENDS_ENABLED | FLAG_OUTPUTS_ENABLED | FLAG_ZSA_ENABLED);
+const FLAG_SWAPS_ENABLED: u8 = 0b0000_1000;


is this compliant with the Swaps zip?

Flag for swaps is not mentioned in the zip, probably should ask Vivek to add it

PaulLaux · 2025-02-19T17:08:47Z

src/domain/orchard_domain_vanilla.rs

+use super::{
+    orchard_domain::OrchardDomainCommon,
+    zcash_note_encryption_domain::{
+        build_base_note_plaintext_bytes, Memo, COMPACT_NOTE_SIZE_VANILLA, NOTE_VERSION_BYTE_V2,
+    },
+};


Should not move existing imports

it's cargo fmt

Preserve Import Order
Add this to your rustfmt.toml:

reorder_imports = false

This prevents Rust from reordering imports.
(This is needed for everything we do.)

PaulLaux · 2025-02-19T17:10:10Z

src/note.rs

    ///
    /// [orcharddummynotes]: https://zips.z.cash/protocol/nu5.pdf#orcharddummynotes
-    pub(crate) fn dummy(
+    pub fn dummy(


why?
generaly we do not want to expose this.
Consider a soution where the dummy padding is happaning inside orchard (add relevent function)

I need dummy notes and keys to build a tx in librustzcash tests

exposing an unneeded api publicly for the sake of external tests will not fly.
Can we reproduce the conntent of dummy() inside librustzcash?

I would need to duplicate a lot, not sure its the best way. Perhaps we play with test-dependencies and features instead, what do you think?

Dummy methods have been copied to librustzcash

just make sure it is inside #[Test]

PaulLaux · 2025-02-19T17:10:42Z

src/tree.rs

 impl MerklePath {
    /// Generates a dummy Merkle path for use in dummy spent notes.
-    pub(crate) fn dummy(mut rng: &mut impl RngCore) -> Self {
+    pub fn dummy(mut rng: &mut impl RngCore) -> Self {


same for dummy()

PaulLaux · 2025-02-19T17:27:38Z

src/bundle.rs

 }

+/// Defines the authorization type of an Orchard bundle with a proof.
+pub trait AuthorizedWithProof: Authorization<SpendAuth = redpallas::Signature<SpendAuth>> {


why do we need a new trait?

Why not add proof() to impl ActionGroupAuthorized { ?

In serailization normal ZSA bundle and ZSA bundle that is inside ActionGroup are almost identical, except Authorization. It's Authorized and ActionGroupAuthorized. To depuplicate serialization I need them both to implement a trait that has a proof() method

not sure I follow.
AuthorizedWithProof adds no new information to the content. There is already a proof inside ActionGroupAuthorized. Why do we need a new generic AuthorizedWithProof again?

Both ActionGroup and OrchardZSA budnles are Bundle<A, V, D>. They are identical, but in first case A=Authorized, in second A=ActionGroupAuthorized. During serialization we need to write a proof which is a part of both, but it is not a part of any common trait, it is just 2 unrelated fields in 2 unrelated structures. So to deduplicate the code I added this common interface that describes something that is Authorized and has proof method

In both cases A is Authorization
let's add fn proof(&self) -> Option<&Proof>; to Authorization and implement it 4 times (InProgress and Unauthorized will return None).

Better than more adding traits with partial intersection.

This reverts commit 07b94c8.

PaulLaux

much much better. Minor comments + fix Ci to merge

Also, please update description of the Swap PR #116 to reflect the changes and the missing Zip detail comment.

PaulLaux · 2025-03-09T07:18:34Z

rustfmt.toml

@@ -0,0 +1 @@
+reorder_imports = false


new line
(consider updating ide setup to be automatic)

PaulLaux · 2025-03-09T07:24:40Z

src/domain/orchard_domain_vanilla.rs

+    zcash_note_encryption_domain::{
+        build_base_note_plaintext_bytes, Memo, COMPACT_NOTE_SIZE_VANILLA, NOTE_VERSION_BYTE_V2,
+    },
+};


any reason moving the imports?

This PR contains a set of additions/changes that allow ZSA Swaps to be implemented in librustzcash: - Adds Swap BundleType and Flag - Adds AuthorizedWithProof trait that defines proof() method for both Authorized and ActionGroupAuthorized - Adds utility methods e.g. from_parts() for SwapBundle, is_empty() in Builder - Changes visibility of some methods e.g. dummy() for testing in librustzcash - Removes ActionGroup structure

alexeykoren added 11 commits February 11, 2025 17:16

Add reference_notes getter

9be2ba2

Add Swap bundle type

a924594

Add Clone to SwapBundle

6aba06f

Add is_empty method

d6fbeb9

Remove reference notes getter

53018a5

Add from_parts to SwapBundle

fef2d18

Add AuthorizedWithProof trait

f1a50e6

Add SpendAuth to AuthorizedWithProof

edcdbe6

Change visibility of dummy methods

07b94c8

Remove redundant bundle type

80b17db

Add Flag and BundleType for swaps

f289651

alexeykoren requested review from ConstanceBeguier and PaulLaux February 19, 2025 09:18

alexeykoren added 2 commits February 19, 2025 10:22

Format

15738e9

Add missing import

4fb1e53

PaulLaux reviewed Feb 19, 2025

View reviewed changes

alexeykoren added 2 commits February 24, 2025 19:00

Revert "Change visibility of dummy methods"

5594483

This reverts commit 07b94c8.

Disable existing imports reordering

3987f5b

QED-it deleted a comment from what-the-diff bot Feb 27, 2025

alexeykoren added 3 commits March 1, 2025 18:50

Remove ActionGroup structure

b92bd14

Move proof method to Authorization

0d64df0

Deduplicate action group hash

695b604

PaulLaux approved these changes Mar 9, 2025

View reviewed changes

alexeykoren added 2 commits March 9, 2025 17:47

Fix formating

0b9a7cb

Fix comments

b7dc5d5

alexeykoren merged commit bd81617 into zsa_swap Mar 9, 2025
28 checks passed

alexeykoren deleted the zsa_swap_spendauth_clone branch March 9, 2025 17:45

		@@ -0,0 +1 @@
		reorder_imports = false No newline at end of file

Conversation

alexeykoren commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PaulLaux 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulLaux Feb 20, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulLaux Feb 20, 2025 • 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!

alexeykoren Feb 20, 2025 • edited by PaulLaux Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulLaux 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexeykoren commented Feb 19, 2025 •

edited

Loading

PaulLaux left a comment •

edited

Loading

PaulLaux Feb 20, 2025 •

edited

Loading

PaulLaux Feb 20, 2025 •

edited

Loading

alexeykoren Feb 20, 2025 •

edited by PaulLaux

Loading

PaulLaux left a comment •

edited

Loading