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

[WIP] Inconsistency fixes for ZIPs 226 and 227 #18

Closed
wants to merge 19 commits into from
Closed

Conversation

AntoineRondelet
Copy link

@AntoineRondelet AntoineRondelet commented May 14, 2023

ZIP 226 and 227 review: Overall comments

  • Better draw the relationship between ZIP 226 and 227: Avoid to force the reader to bounce from 226 to 227 and back. Try to keep the relation between both ZIPs as linear as possible and define key common terms in 227, then refer to 227 from 226 (instead of defining things in both and bouncing from one to another)

    • 227 is the predecessor of 226
    • 226 builds in 227
  • Use consistent capitalization when referring to terms defined in the TERMINOLOGY

  • Change e.g.: to e.g.

  • Make sure orders are all consistent when using tuples: some arguments orders are inconsistent.

  • block chain -> blockchain (everywhere)

    • Fixed in 226
    • Fixed in 227
  • Be consistent with indices over summations.

    • These are missing in many cases: Always specify them explicitly, that removes potential ambiguity and flag potential errors.
    • Use the \sum_{i=1}^n notation instead of \sum_{\forall i \in S} which is heavier and not consistent with the Zcash specs that use the \sum_{i=1}^{n} notation.
  • Shouldn't we use the notations for the group operations defined in 4.1.7.2 of the Specs instead of sums everywhere?

  • Some \forall are unnecessary and can safely be removed to simplify notations

  • Be consistent in the order in which subscripts an hyperscripts are used. e.g. t_{i}^{n} and t^{n}_{i} are equivalent but inconsistent. It's better to have a systematic approach and e.g. always define the subscript first t_{i}^{n}

TODO: Check if we can use latex macros in rst files, this will standardize notations and save a lot of typos.

ZIP 226

  • Page 3:
  • Broken itemize in Backward compatibility section
  • inconsistent notations for AssetBase. As per ZIP-227 an assetID maps to an AssetBase denoted AssetBase^{protocol}_{assetID}, the upper and subscripts are missing many times. To fix.
    • If the notation is too heavy, we can define AssetBase^{protocol} as an alias of AssetBase^{protocol}_{assetID} and also define AssetBase as an alias of AssetBase^{protocol}_{assetID} when the protocol is clear from context, e.g. in ZIPs 226 and 227, it is clear that the protocol that is built upon is Orchard, so it may be worth and clear enough to say that AssetBase = AssetBase^{Orchard}_{assetID} when the notations become too heavy (e.g. when assetBase needs to be used in a subscript or "exponent"). - DONE in ZIP227
  • Page6: in the definition of cv^{net}, the input order to the ValueCommit function isn't consistent compared to bvk in Burn Mechanism section. Use convention: v first, AssetBase second (everywhere)
    • Fixed the tuple order in assetBurn to be consistent with the argument order of ValueCommit
  • Page13: in bvk equality, assetBurn is defined a set (not a vector btw), so we sum over the set. Then do I get it properly that j is the index related to the the set assetBurn? What's the cardinality of this set? (n should correspond to the number of action descriptions with value commitment cv^{net}). I don't get the rcv_{i,j} notation don't we miss a term in the sum over assetBurn? Isn't it smthg like \sum_{j=1}^{|assetBurn|} cv_{j} ValueCommit ....? where |S| is the set cardinality.

ZIP 227 - EDIT: All changes related to ZIP 227 have been moved in a separate PR (#20)

  • Page 1: No need for self reference in abstract (i.e. [6] refers to the actual current ZIP)
  • Page 5: Should purpose be purpose'?
  • Page 7/8: The Size and Bytes columns are inconsistent. Stick to Bytes or Size (in bytes) and make sure only numbers are in the column (remove bytes suffix in column entries)
  • Consolidate most of the terminology section in ZIP227.

vivek-arte and others added 16 commits March 15, 2023 18:33
Co-authored-by: daniben31 <danielbenarroch92@gmail.com>
Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: Jonathan S. Rouach <jon@rouach.net>
Co-authored-by: str4d <thestr4d@gmail.com>
Co-authored-by: Paul <lauxpaul@protonmail.com>
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Includes improvements to mathematical notation, and typographical edits.

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>
This covers the changes made to derive the issuance key independently of the Orchard key structure, using the techniques from [ZIP 32](https://zips.z.cash/zip-0032).
…ations (#13)

Minor changes to the Security and Privacy Considerations
to make it more in line with the format specified in ZIP 0.
This rearranges and rewrites various sections of the ZIP to make it more in line with the suggestions in ZIP 0.
It also updates the Split Notes, Circuit Statement and Burn Mechanism sections with more information.
This adds in the reference links to the test vectors and reference implementations corresponding to the ZSA Protocol. Some corrections to the notation for better consistency and some updates to the formulae for syncing with the implementation are also included here.
@netlify
Copy link

netlify bot commented May 14, 2023

Deploy Preview for zcash-zips-qedit ready!

Name Link
🔨 Latest commit d10337b
🔍 Latest deploy log https://app.netlify.com/sites/zcash-zips-qedit/deploys/6460bec97a12a50008facfd9
😎 Deploy Preview https://deploy-preview-18--zcash-zips-qedit.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@AntoineRondelet AntoineRondelet marked this pull request as draft May 14, 2023 10:25
@vivek-arte
Copy link

I have created #79 to address updates to the use of capitalization for terminology like MUST etc. The other action points seem to have been addressed in subsequent updates, so I am closing this PR for now.

@vivek-arte vivek-arte closed this Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants