Add implementation for stake certificate creation in cardano-wasm internal lib
#1008
Conversation
palas
requested review from
CarlosLopezDeLara,
Jimbo4350,
carbolymer,
dcoutts,
disassembler,
erikd,
kevinhammond and
newhoggy
as code owners
cardano-wasm/src-lib/Cardano/Wasm/Api/Certificate/StakeCertificate.hs
Outdated
Show resolved
Hide resolved
cardano-wasm/src-lib/Cardano/Wasm/Api/Certificate/StakeCertificate.hs
Outdated
Show resolved
Hide resolved
| Exp.obtainCommonConstraints era $ | ||
| conwayEraOnwardsConstraints (convert era) $ | ||
| Certificate . ConwayTxCertDeleg | ||
| <$> ( case (action, delegateStake) of |
There was a problem hiding this comment.
Reading this case gives off a feeling that StakeCertificateObject should be a sum type. The advantage is that you'd have to do more validations at the deserialisation stage, to reject invalid JSON representations and you wouldn't have to check for invalid states here.
There was a problem hiding this comment.
I was having doubts about this. My thought is that it would affect the API and it would be easier to have the API functions be simplest.
So, this way we make the registration/unregistration, deposit, and delegation aspects of the certificate independent, so you can set them one by one.
The disadvantage is, like you say, that we have a type that allows for invalid certificates, and that is disallowed at serialization time.
The advantage is that we don't have the almost cartesian product of possibilities, all of them with pretty much the same arguments:
- Register without deposit (stakeid)
- Register with deposit (stakeid, deposit)
- Unregister (stakeid)
- Unregister with deposit (stakeid, deposit)
- Register with deposit and delegate (stakeid, deposit, poolid)
- Delegate (stakeid, poolid)
Which is also not super nice
cardano-wasm/src-lib/Cardano/Wasm/Api/Certificate/StakeCertificate.hs
Outdated
Show resolved
Hide resolved
cardano-wasm/src-lib/Cardano/Wasm/Api/Certificate/StakeCertificate.hs
Outdated
Show resolved
Hide resolved
| <$> ( case (action, delegateStake) of | ||
| (DelegateOnly, Nothing) -> | ||
| throwError | ||
| "Certificate must at least either: register, unregister, or delegate" |
There was a problem hiding this comment.
I don't understand this error. Why?
There was a problem hiding this comment.
It is not possible to create a certificate that does nothing, it needs to either register stake, or unregister stake, or delegate, or both register and delegate
palas
force-pushed
the
stake-certificates
branch
3 times, most recently
from
3c83b92 to
0e9d62c
Compare
Jimbo4350
left a comment
Jimbo4350
left a comment
There was a problem hiding this comment.
A lot of these functions are not used anywhere. What is the goal here? Where will they be used?
| import Data.Text qualified as Text | ||
| import Data.Text.Encoding qualified as Text | ||
|
|
||
| data StakeCertificateAction |
There was a problem hiding this comment.
Why is StakeCertificateAction not defined like the following:
data StakeCertificateAction
= RegisterStake Delegatee
| UnregisterStake Delegatee
| DelegateOnly Delegatee
There was a problem hiding this comment.
The intention was to be able to have separate withDelegation and withoutDelegation methods, if it is a sum type, then it is less obvious how to implement that, and in some situations withDelegation and withoutDelegation methods would fail. But I am not 100% sure this is the best approach. So maybe we can do something more straightforward even if more verbose
They will all be exposed to the JS API. I was doing this in steps, in case we wanted to rethink the API before adding the boiler-plate. Which seems to be the case |
palas
force-pushed
the
stake-certificates
branch
from
39b15b8 to
6b352f0
Compare
|
I've redone this PR to reuse existing types as much as possible. I have backed up the old approach in this branch for now: https://github.com/IntersectMBO/cardano-api/tree/stake-certificates-old-approach |
| makeStakeAddressRegistrationCertificateWrapper Exp.ConwayEra skHash deposit | ||
|
|
||
| -- | Make a stake address registration certificate in the current experimental era. | ||
| makeStakeAddressRegistrationCertificateExperimentalEraImpl | ||
| :: MonadThrow m => String -> Integer -> m String | ||
| makeStakeAddressRegistrationCertificateExperimentalEraImpl skHashStr deposit = do | ||
| skHash <- readHash skHashStr | ||
| makeStakeAddressRegistrationCertificateWrapper Exp.DijkstraEra skHash deposit |
There was a problem hiding this comment.
ConwayEra and DijkstraEra are hardcoded in multiple places. Could you define those in one place and reuse it? It will be a pain in the neck to trace and update those on hardfork.
Changelog
Context
Work towards achieving support described in issue: #1007.
It is still necessary to expose the functions in the JavaScript API, and also support for adding certificates to transactions.
How to trust this PR
It is mainly a wrapper. I wanted to make it a separate PR to allow for early feedback on the API design.
Checklist