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

framer: includer user-agent in version packet. #17

Closed
wants to merge 2 commits into from

Conversation

chjj
Copy link
Member

@chjj chjj commented May 23, 2014

No description provided.

@chjj
Copy link
Member Author

chjj commented May 23, 2014

Not that important. I just thought we could rep the bcoin.

writeU32(p, 0, 8);
assert.equal(off, 4);
off += writeU32(p, constants.services.network, off);
assert.equal(off, 8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Way too much asserts? ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I added a bunch of asserts just to make sure I didn't break anything. I've been running this as my own client and it seems to work well (no assertion failures, peers respond the same way they always do, etc). I can remove all the asserts now if you want.

@indutny
Copy link
Collaborator

indutny commented May 23, 2014

LGTM

@indutny
Copy link
Collaborator

indutny commented May 23, 2014

All landed, thank you!

@indutny indutny closed this May 23, 2014
@chjj
Copy link
Member Author

chjj commented May 23, 2014

Not sure my last commit made it. It fixes the array size, so instead of Array(86) you'd get Array(86 + this.agent.length) (assuming the user-agent is below 0xfd in length). Oh well, probably not that big of a deal.

@chjj
Copy link
Member Author

chjj commented May 23, 2014

And thank you. :)

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