-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Added /name/upload command definition #3547
Conversation
License: MIT Signed-off-by: Wigy <wigy_opensource_developer@yahoo.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off, Thank you! This is a great feature to add, and something we've been wanting for a while. But I have a few concerns. The first is the naming of this, see the comment about that to discuss what we call this feature.
Second, this is a pretty sizeable PR. I think we can break it up into three logically separate changesets, the first being the refactor of the namesys, the second being the adding of the underlying code to 'republish' a given record, and the third part being the actual command that exposes this functionality.
Also try to avoid cosmetic changes, such as renaming imports or making changes to unrelated files. If you want those things changed, please do make a separate PR for them. Bundling everything together makes it hard to reason about.
Beyond that, I'd love to see some tests for this. We can make something in test/sharness that uses iptb to play with this in a cluster of nodes. Let us know if you need help with that.
} | ||
|
||
pubkey, err := crypto.UnmarshalPublicKey(pubkeyBytes) | ||
crypto.MarshalPublicKey(pubkey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is unnecessary
return | ||
} | ||
|
||
ctx := req.Context() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this closer to where its used
@@ -53,6 +48,7 @@ Resolve the value of a reference: | |||
|
|||
Subcommands: map[string]*cmds.Command{ | |||
"publish": PublishCmd, | |||
"upload": UploadNameCmd, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upload probably isnt the best name for this feature... maybe something like publish-record
could be used, since its not intented as a common thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that record stores will be simple stores with get/put interfaces, something like /record/put
sounds like a good name to me
@wigy-opensource-developer Any update here? |
I was waiting for the 0.4.5 release madness to settle a bit before rebasing, but the current pace is even faster. Wow. My goal is to get this upstream, but I waste quite some time with merging imports that were updated. Excuse me for the slowness. |
@wigy-opensource-developer sounds good. Lets also go with calling this |
@wigy-opensource-developer also, could you PR your sharness and travis.yml changes separately? Smaller PRs == easier for us to review == faster merges |
Roger. I am planning to spend 2 days on it next week. |
@wigy-opensource-developer update here? This would be a really nice feature to have |
Sorry for losing focus on my 2 pull requests, but I am getting back to these soon. Recently I am swapping too many hats. And you are pushing out releases like crazy. Much releases, very impressive. Wow. Doge approved. |
@wigy-opensource-developer do you plan to continue it? |
@Kubuxu Hi Jakub! Without any consensus on #3540 it makes no sense for me to rebase this ancient PR onto the recent codebase. I have also did a responsible disclosure on a security flaw that someone without the private key for an IPNS record could make it impossible for the owner to ever successfully upgrade the record. No answer arrived for that. So however famous IPFS is, the IOP project is implementing our own DHT that can still integrate with IPFS, but does not rely on getting you on the same page in terms of possible use-cases and security questions. It does not leave a bad taste or anger, but we had to move on somehow. Feel free to contact me on how to improve communication with your team, because we still appreciate all the efforts you put into decentralization. |
As part of adding the light client feature, a new command
name upload
is added to the admin interface that allows mobile clients to ask an IPFS node to publish an IPNS record for them. Putting this to the admin interface is an intermediate solution. Authorization (do I let someone with that key that signed this record use my node) is now left to some other process that has access to the admin interface.Renewing (extending EOL) of these records cannot be done by the IPFS node, because the private key is not shared with it. As a consequence, EOL will not be 24hrs from the signing, but about a month from it. TTL is not affected by this limitation and should only depend on the expected frequency of updates to that name.
Republishing the same record periodically on the DHT is missing, like for those that are published with
ipfs name publish --key notself
#3537As part of this change, namesys/republisher now reuses code from the publisher. Also, I have worked on the sharness tests using FUSE, but some are still failing on Linux.