Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Companion for substrate #14036 #7145

Merged
merged 6 commits into from
May 4, 2023
Merged

Companion for substrate #14036 #7145

merged 6 commits into from
May 4, 2023

Conversation

davxy
Copy link
Member

@davxy davxy commented Apr 27, 2023

@davxy davxy requested a review from a team April 27, 2023 17:34
@davxy davxy self-assigned this Apr 27, 2023
@davxy davxy added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Apr 27, 2023
@burdges
Copy link
Contributor

burdges commented Apr 27, 2023

I disagree with renaming VrfTranscript to VrfInput because transcripts are used for thing other than the input.

It's fine to rename some of the variables since they are inputs, but that's not the general situation for the type.

@davxy
Copy link
Member Author

davxy commented Apr 27, 2023

I disagree with renaming VrfTranscript to VrfInput because transcripts are used for thing other than the input.

It's fine to rename some of the variables since they are inputs, but that's not the general situation for the type.

The VrfInput name maybe is not the most technically accurate but has been choosen because wants to represent some kind of (flexible) input to the vrf_sign method and not THE VRF input as you correctly (technically) intend.

From an abstract POV this input can contain (as you mentioned in the substrate PR) other stuff and is specific to the particular vrf.

Indeed in this case VrfInput contains transcript but also extra data.

I don't think that the end user should be interested or be aware of the very technical details (frankly I don't even think that all users know the meaning of a "transcript" and the role it plays in the Fiat-Shamir transform, and probably - at this level - they should not bother).

IMHO they should just have an abstraction to construct "something" to be given to a method to sign and they should be informed which part of this "input type" contributes to the vrf output and which instead just to the proof.

To summarize. IMVHO just Transcript is not the most appropriate because:

  • is not just the transcript
  • the user probably doesn't care
  • doesn't fit well with the generic vrf trait where other vrfs may not use transcripts

BTW, we can also think about other names

@paritytech paritytech deleted a comment from paritytech-cicd-pr Apr 29, 2023
@paritytech paritytech deleted a comment from paritytech-cicd-pr May 4, 2023
@davxy davxy enabled auto-merge (squash) May 4, 2023 14:19
@davxy davxy merged commit 260d26b into master May 4, 2023
@davxy davxy deleted the davxy-companion-for-14036 branch May 4, 2023 14:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants