-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add stake address registration and delegation certificate and stake pool delegation certificate to compatible commands #1070
base: master
Are you sure you want to change the base?
Conversation
2e3e4d7
to
d1bc917
Compare
52daacd
to
48414a5
Compare
859a1fe
to
107da6c
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.
Please break this up into sensible commits
…ompatible commands run in CIO
…mmands. Make them run in CIO instead of ExceptT
107da6c
to
9480b1c
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.
Nice work 👍 . The commits you split the PR into made reviewing your changes much easier. There are a couple of comments, the one regarding catching exceptions should be addressed but I won't block the PR on this.
throwCliError = throwIO | ||
-- | Wrapper function which allows throwing of types of instance `Error`, attaching call stack | ||
-- from the call site | ||
throwCliError :: (HasCallStack, Show e, Typeable e, Error e, MonadIO m) => e -> m a |
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.
IIRC this will return a callstack to this module. It's slightly ugly IMO but I wouldn't block the PR on this.
|
||
fromEitherCli :: (HasCallStack, MonadIO m, Show e, Typeable e, Error e) => Either e a -> m a | ||
fromEitherCli = withFrozenCallStack $ \case | ||
Left e -> throwCliError $ CustomCliException e | ||
Left e -> throwCliError e |
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.
👍
-- TODO This needs to be changed in the future to let the top level exception handler handle the | ||
-- exceptions printing. | ||
newExceptT $ | ||
runRIO () $ |
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.
Why are we catching here? Why not just: runRIO () $ runAnyCompatibleCommand cmd
? toplevelExceptionHandler
will handle the exceptions.
|
||
data CompatibleStakeAddressCmds era | ||
= CompatibleStakeAddressRegistrationCertificateCmd | ||
(ShelleyBasedEra era) |
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.
Some haddocks here explaining for instance why we have Maybe Coin
would be useful.
|
||
let regCert = makeStakeAddressRegistrationCertificate req | ||
|
||
fromEitherIOCli @_ @(FileError ()) $ |
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.
Interesting. We are prevented from specifying @(CIO e)
because it is a type synonym. However @(RIO e)
works.
Changelog
Context
Adds support for more compatible commands
Checklist