-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add support for IPNS #17
Conversation
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.
I left some review comments. Also, a merge from master
is needed.
(sorry, I stated looking at PRs in chronological order and already merged pr/16 before seeing your "merge this and drop 16" message here)
include/ipfs/client.h
Outdated
/** [out] IPNS name Id (multihash) of the named object. */ | ||
std::string* name_id, | ||
/** [in] Lifetime duration of the record. */ | ||
const std::string& lifetime = "24h", |
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.
The resolve
option is missing.
What about passing a Json
object for all of the 4 options described in https://github.com/ipfs/interface-js-ipfs-core/blob/master/SPEC/NAME.md#namepublish? If a property is not present in the supplied Json
object, then we don't pass it to curl.
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.
Yes, that may be a better API. Should it come before, or after the returned value? If after, then it can be given a default value. If before, then not... I;; push a change that places it before, but without a default.
OK, I've made all requested changes, and pushed a new API for |
BTW: The |
This adds support for publishing IPNS entries to IPFS objects.
The IPNS API requires the Key API given in pull req #16. If you merge that first, this will result in a merge conflict; I'll have to rebase. Alternately, merge this, and reject #16.