-
Notifications
You must be signed in to change notification settings - Fork 65
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
Binary Serialization Standard #1478
Binary Serialization Standard #1478
Conversation
@bradjohnl I'm getting a strange failure on the CICD for this one - could you take a look when you get a chance? |
Hey @ACStoneCL . It's fixed. I've removed some hardcoded, and apparently redundant tests that were targeting pages that this PR removed / changed. I've created an internal issue in our SRE board to check the testing strategy and make sure we are already testing each internal and external link with other processes (namely the: node check-external-urls.js test script). |
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 left a few minor nits and a few questions about some methods I don't see anymore. It would be good to address these before approving.
While reviewing this file, a few other questions came up:
- There's a bit of duplication with the json-rpc/types_chain.md file, although the serialization section is much more detailed. Perhaps we can auto-generate the json-rpc docs in the future to avoid double maintenance.
- I see that some types are missing from what we expose in the JSON RPC; perhaps this is because they are not exposed in the RPC methods. Do you know why this the case?
- Some of the fields I noted were: Message, MessageAddr, MessageLimits, MessagePayload, MessageTopicOperation, DeployConfig, TransactionConfig, TransactionHeader, TransactionId, TransactionWithExecutionInfo, BalanceHoldAddr, BidAddr, BlockGlobalAddr, BlockSignatures.
I tried to help resolve a conflict, and then I had to fix a link for the build to pass. |
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.
Approval pending comments from @ipopescu
Co-authored-by: Iulia Popescu <ipopescu@users.noreply.github.com>
Co-authored-by: Iulia Popescu <ipopescu@users.noreply.github.com>
What does this PR fix/introduce?
This PR turns the "Serialization Standard" into the "Binary Serialization Standard" and splits it into multiple pages for ease of reading.
The document itself now includes all updated
Condor
types outside of the chainspec, which will be a separate PR.Closes #1471 #1378
Additional context
// Add further detail on the current situation, or what the PR seeks to address.
// Include loom, screenshots, or gifs(record, compress) if helpful.
Checklist
(Delete any that aren't relevant)
yarn install && yarn run build
.../../faq/faq-general.md
- instead of introducing absolute file path, or relative/absolute URL.yarn run check:externals
.Reviewers
TBD