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

chore: store self protocols in protobook #760

Merged
merged 2 commits into from
Oct 27, 2020

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Sep 24, 2020

This PR stores the self protocols in the Protobook on handle/unhandle.
This allows the identify service to be consistent, by tracking self protocols change in the same way as #748 does for multiaddrs.

Protobook.remove API method was added for the unhandle.

Needs:

@vasco-santos vasco-santos force-pushed the chore/store-self-protocols-in-protobook branch 3 times, most recently from d42da6c to f6ccca0 Compare September 25, 2020 15:35
@vasco-santos vasco-santos force-pushed the chore/store-self-protocols-in-protobook branch from f6ccca0 to a263547 Compare September 25, 2020 16:12
@vasco-santos vasco-santos marked this pull request as ready for review September 25, 2020 16:26
@vasco-santos vasco-santos force-pushed the chore/store-self-protocols-in-protobook branch 2 times, most recently from 628cbce to 24a16df Compare October 7, 2020 16:20
@vasco-santos vasco-santos force-pushed the chore/store-self-protocols-in-protobook branch from 24a16df to bb6e463 Compare October 26, 2020 19:28
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

overall looks good, just some nits in the tests

test/peer-store/proto-book.spec.js Outdated Show resolved Hide resolved
test/peer-store/proto-book.spec.js Outdated Show resolved Hide resolved
if (changeTrigger === 0 && arraysAreEqual(protocols, finalProtocols)) {
defer.resolve()
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

just a nit, you could pass a sinon spy as the event handler, then you could validate the emitted data after each update. Then you dont have to deal with the trigger counter.

// Wait 50ms for incorrect second event
setTimeout(() => {
defer.resolve()
}, 50)
Copy link
Contributor

Choose a reason for hiding this comment

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

spy as the handler would also avoid you needing to use a timeout here

Co-authored-by: Jacob Heun <jacobheun@gmail.com>
@vasco-santos vasco-santos force-pushed the chore/store-self-protocols-in-protobook branch from 106ddfa to 102ccd3 Compare October 27, 2020 13:42
@vasco-santos vasco-santos merged commit c9e6757 into 0.30.x Oct 27, 2020
@vasco-santos vasco-santos deleted the chore/store-self-protocols-in-protobook branch October 27, 2020 13:50
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.

2 participants