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

feat: adding circuit addresses #14

Merged
merged 11 commits into from
Mar 27, 2017
Merged

Conversation

dryajov
Copy link
Member

@dryajov dryajov commented Feb 26, 2017

My attempt at implementing the p2p-circuit addresses, please review. This is still using the /ipfs instead of the /p2p id.

@whyrusleeping @diasdavid @dignifiedquire @lgierth

needs multiformats/js-multiaddr#41

'/p2p-circuit/ip4/1.2.3.4/tcp/3456/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSoooo4',
'/p2p-circuit/ip4/127.0.0.1/tcp/4002/ipfs/QmddWMcQX6orJGHpETYMyPgXrCXCtYANMFVDCvhKoDwLqA',
'/p2p-circuit/ipfs/QmddWMcQX6orJGHpETYMyPgXrCXCtYANMFVDCvhKoDwLqA'
]
Copy link
Member

Choose a reason for hiding this comment

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

it would be good to add some bad cases as well

Copy link
Member Author

Choose a reason for hiding this comment

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

gonna add those shortly.

@dryajov
Copy link
Member Author

dryajov commented Mar 27, 2017

@dignifiedquire @diasdavid @whyrusleeping

I've added support for nested/recursive addresses, as in /p2p-circuit/ipfs/QmOne/p2p-circuit/ipfs/QmTwo, etc...


const Circuit = CircuitRecursive()

const IPFS = or(
Copy link
Member Author

Choose a reason for hiding this comment

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

For reference on circuit addresses take a look at - https://github.com/libp2p/specs/tree/master/relay#addressing. The above seems to take care of most of the cases, but it hasn't been cleaned up, I'll try to do it once I've got everything ironed out.

base('p2p-circuit')
)

const CircuitRecursive = () => or(
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't need to be a function

Copy link
Member Author

Choose a reason for hiding this comment

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

Mm, that's just to make it recursive, the idea is that if a function is encountered during n match, it will be called, which will expand into addidional match conditions.

Copy link
Member Author

Choose a reason for hiding this comment

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

One more thing, the reason it has to be a function, is because I can reference variables that haven't finished being declared yet... I.e. I can reference circuit from it's own declaration block.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I know what your talking about, we can just do () => Circuit when we need recursivity there.

@@ -166,6 +183,10 @@ describe('multiaddr validation', function () {
assertMismatches(mafmt.WebRTCDirect, goodIP, goodUDP, badWS)
})

it('Circuit validation', function () {
assertMatches(mafmt.Circuit, goodCircuit)
Copy link
Member

Choose a reason for hiding this comment

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

Should have tests for mismatches as well.

@dryajov
Copy link
Member Author

dryajov commented Mar 27, 2017

Yep, those are coming shortly.

@daviddias daviddias mentioned this pull request Mar 27, 2017
53 tasks
'/p2p-circuit/ip4/127.0.0.1/tcp/4002/ipfs/QmddWMcQX6orJGHpETYMyPgXrCXCtYANMFVDCvhKoDwLqA',
'/p2p-circuit/ipfs/QmddWMcQX6orJGHpETYMyPgXrCXCtYANMFVDCvhKoDwLqA',
'/p2p-circuit/ip4/127.0.0.1/tcp/20008/ws/ipfs/QmUjNmr8TgJCn1Ao7DvMy4cjoZU15b9bwSCBLE3vwXiwgj/' +
'p2p-circuit/ipfs/QmUjNmr8TgJCn1Ao7DvMy4cjoZU15b9bwSCBLE3vwXiwgj'
Copy link
Member

Choose a reason for hiding this comment

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

@whyrusleeping check this examples, @dryajov pretty much covered them all from @lgierth notes.

@coveralls
Copy link

coveralls commented Mar 27, 2017

Coverage Status

Coverage increased (+0.6%) to 91.111% when pulling 40f8c9f on dryajov:master into 43b4016 on whyrusleeping:master.

@dryajov
Copy link
Member Author

dryajov commented Mar 27, 2017

@diasdavid we should have some bad cases in there now.

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.

4 participants