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
Spec alignment review #1509
Spec alignment review #1509
Changes from 10 commits
74b8960
f406664
d30f859
ca74863
019afbb
cbe3fbe
8678142
5f3754c
85717c8
b1adb11
dabb7a6
f299860
9d00b1c
12d0884
f8177fc
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.
we are not changing the message content, just creating the messageID.
infact in broadcast
i.config.GetNetwork().Broadcast(msgID, msg)
below, msgID is never used, we are using the msgid from the message there.maybe its time to not use separate msgid in the Broadcast?
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.
Oh true. I didn't realise it. Yup, at least for the scope of this PR, in spec we also use msgID in Broadcast, but from the msg itself.
Regarding changing the Broadcast function, we would also need to do a change in spec. Maybe it's not the scope of this PR but we can add it later on
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.
@y0sher Can I align this to spec, to use the ID from the msg?
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.
For now, I'm aligning it. Afterwards, if we find it necessary, we can go back to copying the ID from the state.
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's no special reason to use the msgid from the state, the message is ok.