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(ipns): refactored IPNS package with lean records #339

Merged
merged 3 commits into from
Jun 20, 2023
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jun 7, 2023

Closes #335. Progress towards ipfs/specs#376.

@hacdias hacdias self-assigned this Jun 7, 2023
@hacdias hacdias force-pushed the refactor-ipns branch 4 times, most recently from cacef8e to f94e941 Compare June 7, 2023 19:05
@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #339 (35733f5) into main (198f9bc) will increase coverage by 0.61%.
The diff coverage is 56.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #339      +/-   ##
==========================================
+ Coverage   50.58%   51.19%   +0.61%     
==========================================
  Files         281      280       -1     
  Lines       34043    33442     -601     
==========================================
- Hits        17220    17121      -99     
+ Misses      15033    14589     -444     
+ Partials     1790     1732      -58     
Impacted Files Coverage Δ
coreiface/options/name.go 0.00% <0.00%> (ø)
coreiface/options/namesys/opts.go 74.46% <0.00%> (-6.93%) ⬇️
coreiface/tests/name.go 0.00% <0.00%> (ø)
coreiface/tests/routing.go 0.00% <0.00%> (ø)
examples/gateway/proxy/routing.go 13.51% <0.00%> (+0.52%) ⬆️
gateway/handler_ipns_record.go 18.75% <0.00%> (+0.28%) ⬆️
namesys/publisher.go 49.71% <50.00%> (+0.58%) ⬆️
ipns/validation.go 51.55% <51.55%> (ø)
namesys/republisher/repub.go 57.84% <60.00%> (-0.99%) ⬇️
ipns/pb/record.pb.go 66.11% <66.11%> (ø)
... and 4 more

... and 13 files with indirect coverage changes

@hacdias hacdias force-pushed the refactor-ipns branch 3 times, most recently from 5ce23fc to a451ade Compare June 12, 2023 08:35
ipns/record.go Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Jun 12, 2023

Kubo doesn't build (#339) because it depends on the old delegated routing package (https://github.com/ipfs/go-delegated-routing) because it still supports the wonders of Reframe. Either we fix that package to support this new IPNS package, or (preferred) we completely remove the dependency in Kubo, removing Reframe.

Another option is to simply keep the name of the function RecordKey and change Key to HumanKey. But I don't like that option as it doesn't make it clear. I want the routing-specific key to have "routing" in the function name.

@hacdias hacdias marked this pull request as ready for review June 12, 2023 09:13
@hacdias hacdias requested review from lidel and a team as code owners June 12, 2023 09:13
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@hacdias my understanding is that IPNI no longer uses Reframe endpoints (ipfs/kubo#9823 (comment)), they ask their users to set up /routing/v1 integration, so we can finally remove all Reframe things.

  • some feedback/questions inline

ipns/validation.go Outdated Show resolved Hide resolved
ipns/record.go Outdated Show resolved Hide resolved
ipns/record.go Outdated Show resolved Hide resolved
ipns/record.go Outdated Show resolved Hide resolved
ipns/record.go Outdated Show resolved Hide resolved
ipns/validation.go Outdated Show resolved Hide resolved
ipns/validation.go Outdated Show resolved Hide resolved
ipns/validation.go Outdated Show resolved Hide resolved
ipns/validation.go Outdated Show resolved Hide resolved
@hacdias hacdias force-pushed the refactor-ipns branch 3 times, most recently from 8bf25bd to f165191 Compare June 13, 2023 11:23
@hacdias
Copy link
Member Author

hacdias commented Jun 13, 2023

@lidel I also just discovered we have a coreiface.IpnsEntry. I would like to remove it, but I don't think that's the best idea because it includes the Name field, which we do not have in ipns.Record. Wdyt?

@lidel
Copy link
Member

lidel commented Jun 13, 2023

Re

https://github.com/ipfs/boxo/blob/6f82d77995476b0d85722539230a50a321706ead/coreiface/name.go#LL37C5-L37C5

Notes from 1:1:

  • introduce ipns.Name type and use it instead of IpnsEntry returned by Publish here, as only thing that matters / may be unknown is the actual key (publisher uses pet name like self)
    • make it based on byte[] (we logically decouple ipns names from libp2p peerids; both are byte[] Multiaddrs, but you can use ipns without leaking libp2p types)

@hacdias hacdias force-pushed the refactor-ipns branch 5 times, most recently from 5405b7a to eec505b Compare June 14, 2023 08:07
@hacdias
Copy link
Member Author

hacdias commented Jun 14, 2023

@lidel suggestions implemented

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thanks! Mostly lgtm, some cosmetic changes inline.

Next steps:

ipns/name.go Outdated Show resolved Hide resolved
ipns/name.go Show resolved Hide resolved
ipns/pb/record.proto Outdated Show resolved Hide resolved
ipns/pb/record.proto Show resolved Hide resolved
examples/gateway/proxy/routing.go Outdated Show resolved Hide resolved
ipns/name.go Outdated Show resolved Hide resolved
ipns/name.go Outdated Show resolved Hide resolved
ipns/name.go Outdated Show resolved Hide resolved
ipns/name.go Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Jun 15, 2023

some interop test between boxo/ipns and js-ipns explicitly doing V1+V2 and V2-only

Not sure if that's a blocker or not, but the interop tests you linked are for kubo and js-ipfs. I would assume those should be written once the IPIP is up and/or the js-ipns implementation is updated. At the moment, I think js-ipns cannot validate v2 only records: https://github.com/ipfs/js-ipns/blob/master/src/validator.ts#L33.

@hacdias hacdias force-pushed the refactor-ipns branch 2 times, most recently from d5e141e to c5a71fa Compare June 15, 2023 09:14
@hacdias hacdias requested a review from lidel June 15, 2023 12:10
@lidel
Copy link
Member

lidel commented Jun 15, 2023

Yes, I would block this PR on js-ipns doing the same default + being able to validate the default output.

V2-only is opt-in in Kubo, but boxo/ipns here produces V2 by default.
This is a breaking change, unless js-ipns is marks V2-only as valid, and we had it around for a while.

@hacdias I think we will have less headache if we do what we did in Kubo, and keep this library producing V1+V2 by default like it did before, and making V2-only opt-in for now. The goal here is to allow existence of V2-only, not make it default output of the library.

func processOptions(opts ...Option) *options {
	options := &options{}options := &options{}
	// TODO: produce V2-only records by default after IPIP-TODO ships with Kubo and Helia for at least 6 months
	options.CompatibleWithV1 = true

Once we have IPIP, and implement it in JS, and once we have support for validating V2-only everywhere for a while, then we will make it default, but for now let's keep producing V1+V2.

If we do that, then we can land this PR without waiting for IPIP, as we don't change the default behavior.

@hacdias
Copy link
Member Author

hacdias commented Jun 16, 2023

@lidel I have updated this PR to produce V1+V2 by default (and adapted tests). Kubo PR updated to run with latest code. I can take care of js-ipns and the IPIP. I'll write a note.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thanks @hacdias! I think technically it is ready.
Small nits in comments inline, but once addressed, feel free to merge and bubble up to ipfs/kubo#9932 when you feel Kubo is ready 🙏

ps. I think in cases like this, when we have 40 files modified, fine to just add commits to PR, you dont need to rebase and force push. Smaller deltas make it easier to review too – we then squash at the end during merge.

ipns/pb/Makefile Outdated Show resolved Hide resolved
ipns/errors.go Outdated Show resolved Hide resolved
ipns/errors.go Outdated Show resolved Hide resolved
ipns/errors.go Outdated Show resolved Hide resolved
ipns/errors.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Improvements to the ipns package
2 participants