Skip to content

Feature/peer2peer#30

Open
GaeaKat wants to merge 7 commits intoGigamonkey-BSV:masterfrom
GaeaKat:feature/peer2peer
Open

Feature/peer2peer#30
GaeaKat wants to merge 7 commits intoGigamonkey-BSV:masterfrom
GaeaKat:feature/peer2peer

Conversation

@GaeaKat
Copy link
Contributor

@GaeaKat GaeaKat commented Jul 23, 2022

No description provided.

#include "data/encoding/endian/arithmetic.hpp"
#include "address.hpp"
namespace Gigamonkey::Bitcoin::P2P {
struct NodeAddress {
Copy link
Contributor

Choose a reason for hiding this comment

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

types have lower cases names like "node_address". Does it need node? Could it simply be called "address" ?

data::int32_little timestamp{};
};

class AbstractAddressManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't name things "Abstract..." please just "address_manager"

std::string IpAddress{};
data::uint16_big port{};
data::uint64_little services{};
data::int32_little timestamp{};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would addresses include services and timestamp? That's not an address. Also this supports ipv6 right? We want to do everything on ipv6 in Bitcoin eventually.

#include "data/cross.hpp"
#include "gigamonkey/types.hpp"
namespace Gigamonkey::Bitcoin::P2P {
class AssociationID {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason for this to be an abstract type? Would we ever use any other kind of association ID?

@GaeaKat GaeaKat force-pushed the feature/peer2peer branch from bf5ba07 to 7d9a20c Compare July 27, 2022 13:02
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

Comments