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.

Sign up for GitHub

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

Jump to bottom

Add new key_store interface and two new key stores #243

Closed
wants to merge 2 commits into from
Closed

Add new key_store interface and two new key stores #243

wants to merge 2 commits into from

Conversation

Gustav-Simonsson
Copy link

  • Add new generic key_store interface
  • Add new plaintext key store storing unprotected keys on disk
  • Add new encrypted key store storing encrypted keys on disk
  • Add new entropy mixing function using OS and go runtime sources

Add new key_store interface and two new key stores
16f0a9c 
* Add new generic key_store interface
* Add new plaintext key store storing unprotected keys on disk
* Add new encrypted key store storing encrypted keys on disk
* Add new entropy mixing function using OS and go runtime sources
@obscuren obscuren mentioned this pull request Jan 6, 2015
Closed
4 tasks
@Gustav-Simonsson
Copy link
Author

Do not merge!
This lacks integration with key_manager / wallet layer.
Opened for initial review / feedback of new key_store interface and implementations before working on integrating them with upper layers.

@obscuren obscuren changed the title Add new key_store interface and two new key stores (review) Add new key_store interface and two new key stores Jan 6, 2015
@Gustav-Simonsson
Copy link
Author

Note: the GetEntropyTinFoilHat function is half-joke, half-serious. I don't know if it's really a good idea, but was interested in seeing what hard-to-guess sources of (pseudo) entropy is available from within go.

@obscuren obscuren changed the title (review) Add new key_store interface and two new key stores Add new key_store interface and two new key stores Jan 6, 2015
@obscuren obscuren added the review label Jan 6, 2015
@fjl
Copy link
Contributor

fjl commented Jan 7, 2015

In general I'd love to see simpler names, e.g. FooStore instead of KeyStoreWithBellsAndWhistles.

@fjl
Copy link
Contributor

fjl commented Jan 7, 2015

I'm making all those comments because this is your first Go code submission to the project. Please don't feel offended if you know those things.

Address pull request comments
404fe7c 
* Simplify scrypt constants with const block
* Add key store constructors and make their types private
* Simplify key store and file namings to be less Java Enterprise™
* Change test error logging to use t.Error(err)
* Reduce number of naked returns (just like my ex-gf)
* Simplify file reading path code
@Gustav-Simonsson
Copy link
Author

All comments from @fjl addressed (see commit). The comment about struct literals is moot after adding constructors returning private types.

@Gustav-Simonsson
Copy link
Author

@fjl No offense taken - I'm here to learn :) All good comments. Thanks!

fjl
fjl reviewed Jan 7, 2015
View reviewed changes
crypto/key.go
PrivateKey *ecdsa.PrivateKey
}

type PlainKeyJSON struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

These structs don't need to be exported.

@Gustav-Simonsson Gustav-Simonsson mentioned this pull request Jan 15, 2015
Merged
@Gustav-Simonsson
Copy link
Author

New PR addressing latest comments: #259

@obscuren
Copy link
Contributor

Next time you can just leave the PR open. Any commits made against your own repo will automatically be added in the PR.

Zergity pushed a commit to Zergity/go-ethereum that referenced this pull request Apr 28, 2020
@hadv
dccs: re-apply dccs consensus to master branch (ethereum#243)
0122acd
AusIV pushed a commit to NoteGio/go-ethereum that referenced this pull request Feb 22, 2021
@ziogaschr
Merge pull request ethereum#243 from etclabscore/feat/dev-mode-ethash
028e166 
Add an Ethash Fake sealing option for `—dev` mode, using `—dev.ethash`
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Aug 13, 2021
@geohot
Make syncing the L2 chain ~twice as fast (ethereum#243)
515ea43 
* add memoize to MethodById

* switch to reader writer lock
sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request Jan 4, 2024
@anodar
Merge pull request ethereum#243 from OffchainLabs/export-memdb-errors
0461acc 
Export errNotFound from memorydb to use it in Nitro error checks
atenjin pushed a commit to alt-research/go-ethereum that referenced this pull request Apr 4, 2024
@sebastianst
Merge pull request ethereum#243 from ethereum-optimism/seb/ecotone-ma…
f33198a 
…innet-release

Prepare optimistic Ecotone Mainnet release
luanxu-mxc pushed a commit to MXCzkEVM/mxc-geth that referenced this pull request Sep 2, 2024
@mask-pp
fix(eth): mark anchor transaction in traceBlockParallel (ethereum#243)
8622b2c 
* mark anchor tx

* fix comments
s1na pushed a commit to s1na/go-ethereum that referenced this pull request Dec 2, 2024
@liam-lai
attack recover (ethereum#243)
556f98a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gustav-Simonsson @fjl @obscuren