Skip to content
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

heartbeat + enhanced tests + refactor #57

Merged
merged 25 commits into from
Feb 13, 2019

Conversation

samparsky
Copy link
Contributor

Fixes #17

@samparsky samparsky changed the title [WIP] added post heartbeat message route [WIP] heartbeat Feb 4, 2019
@samparsky samparsky changed the title [WIP] heartbeat heartbeat Feb 4, 2019
@samparsky samparsky force-pushed the heartbeat-message branch 4 times, most recently from 47b1558 to e913097 Compare February 5, 2019 11:13
services/validatorWorker/heartbeat.js Outdated Show resolved Hide resolved
test/integration.js Outdated Show resolved Hide resolved
@samparsky samparsky force-pushed the heartbeat-message branch 2 times, most recently from c10b0c0 to d9c944f Compare February 5, 2019 14:29
@Ivshti Ivshti changed the title heartbeat heartbeat + enhanced tests Feb 9, 2019
const { balances } = res.validatorMessages[0].msg
const fakeBalances = { "publisher": "3" }
// increase the state tree balance by 1
let incBalances = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to increment; the same balances is also a valid state transition, cause it does not violate the OUTPACE rules

Copy link
Member

@Ivshti Ivshti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check out the comments for the tests: neither of the tests you've changed need to increment the balance tree; transitioning from a balance tree to the same tree is valid in OUTPACE

also heartbeat doesn't seem to be:

  1. sent from the leader
  2. checked in the test/integration

@Ivshti Ivshti changed the title heartbeat + enhanced tests heartbeat + enhanced tests + refactor Feb 9, 2019
@samparsky samparsky force-pushed the heartbeat-message branch 3 times, most recently from 06fba82 to a679855 Compare February 11, 2019 17:57
@@ -156,11 +156,14 @@ function isValidatorMsgValid(msg) {
// @TODO either make this more sophisticated, or rewrite this in a type-safe lang
// for example, we should validate if every value in balances is a positive integer
return msg
&& typeof(msg.stateRoot) === 'string' && msg.stateRoot.length == 64
&& (
(msg.stateRoot && typeof(msg.stateRoot) === 'string' && msg.stateRoot.length == 64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msg.stateRoot check here is redudant - if it's a string and the length is 64, it's certainly truthy

const producer = require('./producer')
const { heartbeat } = require('./heartbeat')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be const heartbeat = require('./heartbeat') and heartbeat should directly expose heartbeat, to follow the convention set by producer, leader, follower

.then(function(res){
// send heartbeat
if(res && res.nothingNew){
Promise.resolve(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new lines here are redundant IMO,

but most importantly, why is heartbeat not returning a promise?

@@ -58,7 +68,9 @@ function onNewState(adapter, {channel, balances, newMsg, approveMsg}) {
return { nothingNew: true }
}
})
.then(function(){
.then(function(res){
if(res && res.nothingNew) return res
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems bad, but it can be mitigated by just deleting the second .then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just remove lines 70, 71

@@ -58,7 +68,9 @@ function onNewState(adapter, {channel, balances, newMsg, approveMsg}) {
return { nothingNew: true }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also the res here from verify() should be renamed to isValid

})
}

module.exports = { heartbeat }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just module.exports = heartbeat as the convention from follower/leader/etc.

services/validatorWorker/leader.js Outdated Show resolved Hide resolved
test/integration.js Show resolved Hide resolved
test/integration.js Show resolved Hide resolved
test/integration.js Show resolved Hide resolved
.then(function(res){
// send heartbeat
if(res && res.nothingNew){
heartbeat(adapter, channel)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be return heartbeat, right?

@Ivshti Ivshti merged commit a0c557b into AmbireTech:master Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants