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

docs: fix specs #149

Merged
merged 6 commits into from
Mar 27, 2024
Merged

docs: fix specs #149

merged 6 commits into from
Mar 27, 2024

Conversation

karmacoma-eth
Copy link
Contributor

This covers different kinds of edits, from no-brainers to maybe controversial, so I separated them into individual commits for easier inspection.

  • cee81d9 fix table of contents: the links were broken, which is why I started this PR in the first place, should be a no brainer
  • 405adf5 use checksum address: similar, checksum addresses are just better
  • 2972ac3 and 67b0fd9 are purely syntactic changes

Will call out the riskier changes inline.

Additionally, some open ended feedback (feel free to redirect the conversation):

  • there are multiple references to "the Signer CRDT", but it's not defined in the spec
  • I think it would make sense to define in one place what a valid fid is, and then say "field X must be a valid fid". There are references to "an fid is only valid if it is present in the Id registry" (what does this mean exactly?), something being "a known fid" (is it the same as being in the Id registry?), and some references to an fid being either >=0 or >0 (does this mean that a valid integer is ok even if it is not in the Id registry?)

Seems like this was a mistake and should instead be USERNAME_TYPE_FNAME, consistent with protobufs/schemas/username_proof.proto
The spec for CastId states that `c.fid` is an integer > 0, which seems like the correct rule for being a valid fid.
@@ -144,7 +144,7 @@ message MessageData {
A MessageData `data` in a Message `m` must pass the following validations:

1. `m.data.type` must be a valid MessageType.
2. `m.data.fid` must be an integer >= 0.
2. `m.data.fid` must be an integer > 0.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️ calling this out: this is consistent with CastId defining fid as > 0. If the intention here is that fid can be explicitly 0, I think it should be stated otherwise it just looks inconsistent.

Also: does it mean that any fid is valid even if they are not in the Id registry?

@sds sds merged commit cac7e8d into farcasterxyz:main Mar 27, 2024
1 check passed
@sds sds added the documentation Improvements or additions to documentation label Mar 27, 2024
@sds
Copy link
Member

sds commented Mar 27, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants