-
Notifications
You must be signed in to change notification settings - Fork 631
[CDEC-509] Remove HasGeneratedSecrets #3437
Conversation
6dc8c08
to
8d858b2
Compare
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.
LGTM!
auxx/Main.hs
Outdated
printAction "Mode: with-config" | ||
CLI.printInfoOnStart aoCommonNodeArgs ntpConfig txpConfig | ||
let generatedSecrets = fromMaybe | ||
(error "Main.action: GeneratedSecrets missing from config") | ||
(configGeneratedSecrets coreConfig) |
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.
Can we put a bang on this? I'd rather this blow up ASAP than let it propagate the _|_
all the way down until the parameter is eventually evaluated.
Alternatively, do it in IO:
generatedSecrets <- maybe (throwIO (userError ...)) return (configGeneratedSecrets coreConfig)
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.
Ah, good point!
I think I'll extract this into a getGeneratedSecretsThrow :: Core.Config -> IO GeneratedSecrets
. What's the best way to add location information to that?
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.
foo :: HasCallStack => Core.Config -> IO GeneratedSecrets
though I usually find HasCallStack
to be less useful than you want (if the constraint isn't carried in the call tree then it's dropped)
auxx/src/Command/Proc.hs
Outdated
forM_ is $ \i -> do | ||
key <- evaluateNF $ secrets !! i | ||
key <- evaluateNF $ secretKeys !! i |
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.
oof, i know this is just a refactor, but it'd be real nice to have a non partial index here
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.
Wow, not looking at the rest of the code, but is that not just:
forM_ secretKeys $ \k -> do
key <- evaluateNF k
was that written by a C/C++/Java programmer?
--command "stack ghci cardano-sl-chain:lib cardano-sl-chain:test:test --ghci-options=-fobject-code" \ | ||
--test "Main.main" | ||
|
||
.PHONY: ghcid ghcid-test help |
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.
❤️
6c47f85
to
897a65f
Compare
897a65f
to
06b4f09
Compare
[CDEC-509] Remove HasGeneratedSecrets
…hk/ruhatch/CDEC-509 [CDEC-509] Remove HasGeneratedSecrets
Description
Remove the
HasGeneratedSecrets
constraint and replace it with explicit parameter passing ofGeneratedSecrets
Linked issue
CDEC-509
Type of change
Developer checklist
Testing checklist
QA Steps
Screenshots (if available)