-
Notifications
You must be signed in to change notification settings - Fork 401
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
ICS20-1 cleanup #576
ICS20-1 cleanup #576
Conversation
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.
ACK, pending @colin-axner 's clarification.
Thanks for the fix @ethanfrey
ics20 clarification
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.
ACK but see comments - thanks for the improvements!
@milosevic Want to take a quick look at this or shall we merge (already 2/3 approvals)? |
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.
Really nice work.
Merged with 3/3. Thank you @ethanfrey! |
When preparing to add ICS20-2 documentation, I realized the spec deviated from the implementation, and a few items were underspecified.
In particular:
source
field in the packet, but important info is encoded indenomination
, which is not clearly specified in human words (just pseudocode).success: boolean
)/
that is present in the implementation (and correct in the implementation I believe)I added these suggests on a "best effort" basis and if the core devs disagree with any of the description, I would ask them to kindly correct this PR. In any case, this PR should be "more correct" than the current master.