Embedded key loader #82
Conversation
- NewEmbeddedKeyLoader with embedded verification keys for:
- authV2.json
- credentialAtomicQueryMTPV2.json
- credentialAtomicQueryMTPV2OnChain.json
- credentialAtomicQuerySigV2.json
- credentialAtomicQuerySigV2OnChain.json
- credentialAtomicQueryV3-beta.1.json
- credentialAtomicQueryV3OnChain-beta.1.json
- linkedMultiQuery10-beta.1.json
- Updated readme
- added configuration for NewEmbeddedKeyLoader - added examples for supported networks - minor improvements and links to the resources added
README.md
Outdated
| } | ||
|
|
||
| resolvers := map[string]pubsignals.StateResolver{ | ||
| "polygon:mumbai": resolver, |
There was a problem hiding this comment.
polygon:mumbai is not supported, could you update to polygon:amoy
loaders/embededKeyLoader.go
Outdated
| type EmbeddedKeyLoader struct { | ||
| keyLoader VerificationKeyLoader | ||
| cache map[circuits.CircuitID][]byte | ||
| cacheMu sync.RWMutex |
There was a problem hiding this comment.
https://pkg.go.dev/sync#RWMutex
A RWMutex must not be copied after first use.Use pointer to sync.RWMutex to prevent copy in methods.
| if e.keyLoader != nil { | ||
| key, err := e.keyLoader.Load(id) | ||
| if err == nil { | ||
| if e.useCache { |
There was a problem hiding this comment.
move to deffer to prevent copy-past
There was a problem hiding this comment.
You can set the deffer after cache check to not perform the deffer if the keys are found in the cache
| }) | ||
| } | ||
|
|
||
| func TestDefaultEmbeddedKeys(t *testing.T) { |
There was a problem hiding this comment.
golang table driven tests
https://dev.to/boncheff/table-driven-unit-tests-in-go-407b
| loader := NewEmbeddedKeyLoader() | ||
| _, err := loader.Load("non-existent-circuit") | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "failed to load default key") |
There was a problem hiding this comment.
Compare error text is bad practice. Try to use assert.ErrorsIs or assert.ErrorAs
loaders/embededKeyLoader.go
Outdated
| type EmbeddedKeyLoader struct { | ||
| keyLoader VerificationKeyLoader | ||
| cache map[circuits.CircuitID][]byte | ||
| cacheMu sync.RWMutex |
There was a problem hiding this comment.
Consider to use https://pkg.go.dev/sync#Map
loaders/embededKeyLoader_test.go
Outdated
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| key := loader.getFromCache(testID) |
There was a problem hiding this comment.
In my opinion, this test is not useful. Since you are testing parallel reads without write operations, there is no concurrency issue.
| @@ -19,5 +20,9 @@ type FSKeyLoader struct { | |||
|
|
|||
| // Load keys from embedded FS | |||
| func (m FSKeyLoader) Load(id circuits.CircuitID) ([]byte, error) { | |||
| return os.ReadFile(fmt.Sprintf("%s/%v.json", m.Dir, id)) | |||
| file, err := os.ReadFile(fmt.Sprintf("%s/%v.json", m.Dir, id)) | |||
There was a problem hiding this comment.
Possible attack: https://rowin.dev/blog/preventing-path-traversal-attacks-in-go
In your case, the path and filename are constants. It is secure. But make sure that the security issue is not reproduced in your implementation.