-
Notifications
You must be signed in to change notification settings - Fork 457
Use encoded communication in p2p using lisk-codec - Closes #5269 #5423
Conversation
…/LiskHQ/lisk-sdk into 5269_update_liskp2p_encoded_data * '5269_update_liskp2p_encoded_data' of https://github.com/LiskHQ/lisk-sdk: Remove Schema import in P2P from codec
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.
Few minor comments but other than @shuse2 review, LGTM
success: true, | ||
peers: [], |
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.
Does the order of object property matter here
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.
no, it doesn't
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.
Ok, I saw that you changed it so I wondered if there was a reason.
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.
ah yeah, I changed it because I thought it was the reason for failing test :(
sharedState: { | ||
version: '1.1.1', | ||
}, | ||
sharedState: {}, |
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.
No longer including version?
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.
now we only have protocolVersion
, version was to support older nodes. We can remove version
and other fields in another PR. Issue related to it #5419
// import { P2P } from '../../src/index'; | ||
// import { createNetwork, destroyNetwork } from '../utils/network_setup'; | ||
|
||
// TODO: Skipping as strict schema doesn't allow custom fields. Enable this test when users will be allowed to pass custom schema. |
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.
Instead of commenting out, you can simply .skip the test
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 I skip eslint throws an error and even with eslint-disable
it's not working. It works on Mocha
|
||
// Skipping as schema validation doesn't allow custom fields and supported properties are validated before applying nodeInfo. |
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.
Instead of commenting out, you can simply .skip the test
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.
same for this
…/LiskHQ/lisk-sdk into 5269_update_liskp2p_encoded_data * '5269_update_liskp2p_encoded_data' of https://github.com/LiskHQ/lisk-sdk: ♻️ Update variable name 🐛 Fix decoding with nested array with same field numbers ♻️ Refactor chain transaction handler to be first degree function and remove unnecessary functions and behavior
What was the problem?
This PR resolves #5269
How was it solved?
nodeInfo
,peerInfo
, andpeerListResponse
schemaHow was it tested?
Run
npm t
underelements/lisk-p2p