Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: Tangle Message #17
RFC: Tangle Message #17
Changes from 21 commits
3357004
805758f
fa24ebb
c778814
915cd2b
b973ccf
5803db6
4ce1327
04b35a7
67c8004
9d1f832
8ee6316
2caf731
a426446
5eabf5e
7b003e2
385abff
35b8653
c795980
54b0a0a
1150c72
2a43097
cc15a51
3d92d83
793378d
dcbf444
8a69a4c
0e5d060
98e7437
e323b18
dd826e0
5843c38
d3a46d9
d650bb8
ecb8b66
92a2f73
3b330c1
f4f94cd
4e9ecdd
f1b5775
466d328
d4eea48
2ef7705
e5d02e2
1ba4de5
d479e9e
c3ace55
5168461
f641685
74a46d1
b687e55
2e2ef6f
92cc7ef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we gain by using a varint here? Are you planning to have megabyte-sized messages? Make it a uint32 and reduce complexity instead of making even the header of the message variable in size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wanted to not have too many types in the message.
The idea is that less types means more simple descriptions and code.
Also, for lots of cases where we describe quantities
uint8
oruint16
could have sufficed.So the idea is that choosing varints will result mostly in shorter messages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced it will be simpler code. It's not native primitive in many languages.
So we either implement one ourselves or use the existing one. Both would end up with more validation rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
uint8_t
oruint16_t
is sufficed, why not just use one of them? they are primitive types in most languages. Thevarint
is good for data serialization but it is not easy to operate and memory management in a data structure, especially for microcontrollers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
due to recent conversations outside of github I am also leaning towards dropping varints
this decision will be finalized on the next protocol team meeting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: I would rather set fixed reasonable-sized types with some slack (you never know) replacing most varints.
varints
are somewhat complex types to parse, but I would like to draw your attention to the creation of such message rather than the parsing of it.varints
type force the entire message structure to "shift" instead of having fixed offsets. This is quite cumbersome when forming a message where all the parts are not necessarily known before-hand, and therefore the byte-requirements of lengthvarint
types is also unknown: when you fill in the data section of the message, you may see yourself push it forward in order to accommodate the correspondingvarint
size without overflowing into the data section itself. Sure, modern programming languages and libs provide abstract structures for all these, but at a cost of memory reallocation, memory concatenation and other forms of memory juggling. Leveling the participation field ultimately accounts for the flexibility of a solution.