Skip to content

First round of modifications on ZIP227#20

Merged
vivek-arte merged 16 commits into
zsa1from
zip-0227-ar-first-round
May 29, 2023
Merged

First round of modifications on ZIP227#20
vivek-arte merged 16 commits into
zsa1from
zip-0227-ar-first-round

Conversation

@AntoineRondelet
Copy link
Copy Markdown

@AntoineRondelet AntoineRondelet commented May 28, 2023

As per the title, this PR does a first round of changes on ZIP 227.
Each commit does one change, so we can easily cherry pick/rebase to keep the relevant ones.

@netlify
Copy link
Copy Markdown

netlify Bot commented May 28, 2023

Deploy Preview for zcash-zips-qedit ready!

Name Link
🔨 Latest commit 33ca273
🔍 Latest deploy log https://app.netlify.com/sites/zcash-zips-qedit/deploys/64746008f57e220008c03fd7
😎 Deploy Preview https://deploy-preview-20--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.

Comment thread zip-0227.rst Outdated
Comment thread zip-0227.rst Outdated
2 assetDescSize byte The length of the asset\_desc string in bytes
Varies asset\_desc byte UTF-8 encoded string, of size assetDescSize bytes
Varies nNotes compactSize The number of notes in the issuance action
TBD noteSize byte The size, in bytes, of a Note
Copy link
Copy Markdown
Author

@AntoineRondelet AntoineRondelet May 28, 2023

Choose a reason for hiding this comment

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

As per: #20 (comment) it'd be nice to document the noteSize

Comment thread zip-0227.rst

- check that :math:`0 < \mathsf{assetDescSize} <= 512`.
- check that :math:`\mathsf{asset\_desc}` is a string of length :math:`\mathsf{assetDescSize}` bytes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

asset_desc is defined as "UTF-8 encoded string, of size assetDescSize bytes" in the table. Yet, it's Byte size is set to Varies and its type is set as byte.

It seems like it's Byte size shouldn't be Varies but assetDescSize instead and it's type should be a byte array byte[assetDescSize]

Comment thread zip-0227.rst Outdated
Comment thread zip-0227.rst Outdated
2 assetDescSize byte The length of the asset\_desc string in bytes
Varies asset\_desc byte UTF-8 encoded string, of size assetDescSize bytes
Varies nNotes compactSize The number of notes in the issuance action
TBD noteSize byte The size, in bytes, of a Note
Copy link
Copy Markdown
Author

@AntoineRondelet AntoineRondelet May 28, 2023

Choose a reason for hiding this comment

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

As per: #20 (comment) it'd be nice to document the noteSize

@AntoineRondelet
Copy link
Copy Markdown
Author

I'm a bit lost on notations (especially the use of math fonts). We seem to be using mathsf which makes sense, but sometimes we use the double backtick or simply backticks (e.g. for 'finalize' and 'notes').
(see screenshot enclosed).

Is there a convention somewhere?

fonts

@vivek-arte
Copy link
Copy Markdown

vivek-arte commented May 29, 2023

I'm a bit lost on notations (especially the use of math fonts). We seem to be using mathsf which makes sense, but sometimes we use the double backtick or simply backticks (e.g. for 'finalize' and 'notes'). (see screenshot enclosed).

Is there a convention somewhere?

fonts

Looking at other ZIPs (ZIP 225 for example), it seems that terms that are entries in transaction format tables are always denoted via double backticks (with the use of \mathtt if they appear in a math equation).

Copy link
Copy Markdown

@vivek-arte vivek-arte left a comment

Choose a reason for hiding this comment

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

Approved with comments, see above.

@vivek-arte vivek-arte merged commit 18c1e8d into zsa1 May 29, 2023
@AntoineRondelet
Copy link
Copy Markdown
Author

AntoineRondelet commented Jun 4, 2023

Thanks @vivek-arte, but then, why is notes used with single backticks? (same for IssueBundle, IssueAuthSig etc)
Likewise, AFAICT, finalize is not a transaction field in itself, but rather the LSB of flagIssuance, so shall it be double back ticked? Feels like flagIssuance should be but not finalize. What do I miss?

@vivek-arte
Copy link
Copy Markdown

You're right, the use of single backticks was confusing and unnecessary. I've tried to improve on those parts in PR#28, moving to double backticks for the table items.

The finalize is a tricky case for the reason you mentioned, there was a time when we didn't have a full byte and only had the bit in, which is why it originally was in double backticks. That was changed to using the flagIssuance for serialization purposes, but I didn't edit the finalize formatting.

I'm also wondering how strict we need to be about using double backticks only for table entries -- for example, in the above PR, I changed ik to use double backticks because we use it in the issuance bundle table. But then it felt jarring to have isk, the private key that corresponds to ik, in \mathsf format. So I changed isk to also use double backticks. Open to suggestions on how to handle this.

I'm not sure there is a single consistent choice throughout all the prior ZIPs, but if we pick a strict format and stick to it that would be good too. We should document it somewhere, probably in the issue #25 discussion. I'm not sure we should include it in the Rationale section of either ZIP, but we can bring it to the attention of the ZIP editors, and they could choose if they want to make these conventions uniform going ahead and so on.

@vivek-arte
Copy link
Copy Markdown

As an update, also see this comment made by Daira on the isk change I was unsure about. That seems to give clarity on the convention we should use (and means isk should be moved back to mathsf).

PaulLaux pushed a commit that referenced this pull request Oct 4, 2023
Improved style and content of ZIP 227.

---------

Co-authored-by: Vivek Arte <vivek@qed-it.com>
vivek-arte added a commit that referenced this pull request Feb 12, 2024
Improved style and content of ZIP 227.

Co-authored-by: Vivek Arte <vivek@qed-it.com>
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