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

basic keystore implementation #3472

Merged
merged 3 commits into from
Dec 6, 2016
Merged

basic keystore implementation #3472

merged 3 commits into from
Dec 6, 2016

Conversation

whyrusleeping
Copy link
Member

Thank you to @kyledrake for prompting me to just write this already

License: MIT
Signed-off-by: Jeromy [email protected]

License: MIT
Signed-off-by: Jeromy <[email protected]>
@haadcode
Copy link
Member

haadcode commented Dec 6, 2016

Oooh! Excellent stuff! Big fan of your work J :)

Thank you @kyledrake for prompting @whyrusleeping to just write this already.

kp := filepath.Join(ks.dir, name)

_, err = os.Stat(kp)
if err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

To be safe I would use os.IsExist and change it to ErrKeyExists then, else passthrough the error.

Copy link
Member Author

Choose a reason for hiding this comment

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

How would you use os.IsExist here?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, IsNotExist

Copy link
Member Author

Choose a reason for hiding this comment

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

ah! okay, i see what you want

}
defer fi.Close()

_, err = fi.Write(b)
Copy link
Member

Choose a reason for hiding this comment

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

I thought you wanted to use multicodec so we can migrate much easier?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, it won't make migrating easier at all. (it will make them hard since i'll have to decode multicodecs in the migrations code :P )

@Kubuxu
Copy link
Member

Kubuxu commented Dec 6, 2016

Also there are absolutely no tests for it.

@whyrusleeping
Copy link
Member Author

Also there are absolutely no tests for it.

Yeah yeah, i'm getting there.

License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping whyrusleeping merged commit 60371a5 into master Dec 6, 2016
@whyrusleeping whyrusleeping deleted the feat/keystore branch December 6, 2016 23:10
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Dec 6, 2016
@ghost ghost mentioned this pull request Dec 23, 2016
@wigy-opensource-developer
Copy link

wigy-opensource-developer commented Dec 23, 2016

@whyrusleeping I love the new feature, thanks for implementing it.

What is missing is the renewal of the IPNS records every 4 hours. The namesys/republisher uses a so-called keybook in the peerstore that only contains the self key. Probably the republisher needs to use the keystore, too, to enumerate all IPNS records which need a renewal.

As I currently understand, the republisher/reprovider is the only part of the network that fixes data loss caused by peers going online/offline. If there is no automatic recovery from the shifting responsibilities in the DHT layer, in the current state of the code, the ipns name publish --key notself /ipfs/QmBlah needs to be executed regularly as a workaround to make sure the name can be resolved.

@whyrusleeping
Copy link
Member Author

whyrusleeping commented Dec 26, 2016 via email

@Kubuxu
Copy link
Member

Kubuxu commented Dec 26, 2016

I have opened it already: #3537

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