FillDBWithParticipationKeys return object on error #3184
Conversation
enhance comment for Signer struct fix ordering of imports in keystore_test FillDBWithParticipationKeys return on error changes
Codecov Report
@@ Coverage Diff @@
## feature/dilithium-scheme-integration #3184 +/- ##
========================================================================
- Coverage 43.80% 43.76% -0.05%
========================================================================
Files 403 403
Lines 89955 89954 -1
========================================================================
- Hits 39405 39364 -41
- Misses 44270 44299 +29
- Partials 6280 6291 +11
Continue to review full report at Codecov.
|
| if err != nil { | ||
| return PersistedParticipation{}, err | ||
| if err == nil { | ||
| err = stateProofSecrets.Persist() // must be called after part.Persist() |
There was a problem hiding this comment.
If Persist() function is not written well, you will return a PersistedParticipation struct which might be filled with wrong data. I think, In order to protect ourselves, we should return empty struct.
for the err value- this change is great.
There was a problem hiding this comment.
The expectations should be that if error is returned, there are no guarantees on anything else returned (this was something I fixed on master).
When you say "protect ourselves", what are the risks? Maybe, we should make sure Persist is safe when it returns an error.
I have no problem with returning PersistedParticipation{} in case of an error. However, if there are risks with returning part, we need to understand that,
| } | ||
|
|
||
| err = stateProofSecrets.Persist() // must be called after part.Persist() ! | ||
|
|
||
| return part, err |
There was a problem hiding this comment.
notice here, without my change, in the case of stateProofSecrets.Persist returning an error, it is still returning part with err not nil.
If we want to return PersistedParticipation{} in case of an error, we need to check err in two places. I think this will unnecessarily complicate the code.
|
small request - when you're creating a pr, could you please try and give the PR a more informal title ? |
algonautshant
changed the title
Summary
enhance comment for Signer struct
fix ordering of imports in keystore_test
FillDBWithParticipationKeys return on error changes
Test Plan