Certs support in decode transaction #3028

paweljakubas · 2021-11-19T15:17:05Z

Support in tx for certificates:

delegation certs belonging to a given wallet -> then also reward account's derivation path is added
delegation certs not belonging to a given wallet
pool registration/deregistration certs
genesis cert
MIR certs
I have extended support in basic types and in shelley for all certificates
I have extended decodeTx to account for certificates
I have extended ApiDecodedTransaction for "certificates" and added all needed bits in swagger
I have supported extended ApiDecodedTransaction in Api.Server.Types
I have implemented this support in Api.Server's decodeTransaction
I have added illustrative integration tests

Comments

When constructTx will have delegation support we will need to check ours delegation certs in txs. For now txs with external delegation certs (in the context of a given wallet) are checked in integration testing. Also the detection of txs with pool registration/deregistration, and mir certs is tested

Issue Number

Anviking

Nice. Left a few comments. Mainly:

I think we really should preserve the order of the certificates
Perhaps we should use the "certficiate_type" tag consistently for all cert types

Anviking · 2021-12-03T12:04:11Z

lib/core/src/Cardano/Wallet/Primitive/Types.hs

+instance NFData NonWalletCertificate
+
+type Certificates =
+    ([DelegationCertificate], [PoolCertificate], [NonWalletCertificate])


I'd suggest a Certificate sum type instead.

When you re-assemble the three lists in the Server module, you currently end up changing the order of the certificates, which seems like an unnecessary thing. It prevents both roundtrips and users from finding the cert_ix required to create a pointer address.

Although I guess preserving the order might require a bit of re-plumbing all the way from Shelley.Compatibility 🤔

Anviking · 2021-12-03T12:11:17Z

lib/core/src/Cardano/Wallet/Api/Types.hs

+        parseDelOur obj = do
+            derPathM <-
+                (withObject "WalletDelegationCertificate" $
+                 \o -> o .:? "reward_account_path" :: Aeson.Parser (Maybe (NonEmpty (ApiT DerivationIndex)))) obj


I see that many of the certificate types contain the field certificate_type, but not all.

If you ensured all types had it, e.g. replacing "mir" with {"certficiate_type": "mir" }, I believe you could rely on first parsing the type and then case-switching here. This might produce nicer error messages, and be easier to implement for both us and clients.

What do you think?

Could we not have something more like

parseJSON = withObject "ApiAnyCertificate" $ \o -> do (ty :: String) <- o .: "certificate_type" case ty of "register_pool" -> StakePoolRegister <$> parseJSON (Object o) "join_pool" -> WalletDelegationCertificate <$> parseJSON (Object o) "quit_pool" -> WalletDelegationCertificate <$> parseJSON (Object o) ... _ -> fail $ "unknown certificate_type: " <> show ty

I think relying on case over <|> provides some more sanity when `certificate_type´ is present anyway. Even if this code is only for roundtrips and not used from the API.

Anviking · 2021-12-03T12:30:38Z

lib/core/src/Cardano/Wallet/Api/Types.hs

@@ -1171,6 +1176,47 @@ data ApiTxOutputGeneral (n :: NetworkDiscriminant) =
      deriving (Eq, Generic, Show, Typeable)
      deriving anyclass NFData

+data ApiExternalCertificate (n :: NetworkDiscriminant)
+    = RegisterRewardAccountExternal
+        { rewardAccount :: !(ApiT W.RewardAccount, Proxy n)


Did you consider merging ApiExternalCertificate and ApiCertificate by adding a isOurs :: Bool, and deriving "certificate_type" based on both the constructor and the isOurs?

But your approach is also neat in another way. If we ever wanted to extend the "ours" type with more information (e.g. DerivationPath), it would be straight forward.

But one could also imagine:

data ApiCertificate = ApiCertificate RewardAccount ApiDelegationCertKind (Maybe DerivationPath) -- Nothing means external ApiDelegationCertKind = Register | Join pool | Quit

^^^ Some thoughts. Feel free to ignore. And it doesn't affect the API itself.

I feel having them separate is clearer to be honest. But this is not strong feeling;-) Feel free to push commit if you think it should be like you propose

piotr-iohk · 2021-12-07T08:35:17Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+                \e02834b275885e06ed04869747cff43cec91e01b000000e8d4a51000ff9fff\
+                \f6" :: Text
+
+        let cborHexMIR = fromTextEnvelope cborHex


Decoding this by hand; certificates look a bit strange:

$ curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/transactions-decode -d '{"transaction":"84a50081825820e6f53fa3a753f637c62ee198421bbaef604faee230fe4262340eedf13a8d0bba00018182581d61161a20f92ea667c30de21e49f01cc4ba7aadaa9b685a3dab7d4b040a1a000f4240021b00038d7ea3678c40031a3b9ac9ff049f82008200581cfb3c13a29d3798f1b77b47f2ddb31c19326b87ed6f71fb9a27133ad582068200a18200581cfb3c13a29d3798f1b77b47f2ddb31c19326b87ed6f71fb9a27133ad51b000000e8d4a5100082008200581ceb220e40c3ca1de87c448972443020d8fa08d111a699a4d7b51ba4bc82068200a18200581ceb220e40c3ca1de87c448972443020d8fa08d111a699a4d7b51ba4bc1b000000e8d4a5100082008200581cc72a6827138da1341c9ea1e55a04bd1096824f0c66a0ec282d5daad382068200a18200581cc72a6827138da1341c9ea1e55a04bd1096824f0c66a0ec282d5daad31b000000e8d4a5100082008200581c3b525e261b6434b75f949404fbc5b3ef4acf686a31af9facea74687082068200a18200581c3b525e261b6434b75f949404fbc5b3ef4acf686a31af9facea7468701b000000e8d4a5100082008200581c8bf7bb98dd01953c5754fcf49460aaaa3368faf9476267411a38d33c82068200a18200581c8bf7bb98dd01953c5754fcf49460aaaa3368faf9476267411a38d33c1b000000e8d4a5100082008200581c917d3b19a2c9fe13ca2dea4c9b1555257af2f185b58ad4837e801c1782068200a18200581c917d3b19a2c9fe13ca2dea4c9b1555257af2f185b58ad4837e801c171b000000e8d4a5100082008200581c37457aadf2fa6292ebca8460e01ff4e813a495466512b930eb564ec782068200a18200581c37457aadf2fa6292ebca8460e01ff4e813a495466512b930eb564ec71b000000e8d4a5100082008200581cc1e7e3ea91ee7a92f0b33afceab00a41c7039bf083636689d2de1f0482068200a18200581cc1e7e3ea91ee7a92f0b33afceab00a41c7039bf083636689d2de1f041b000000e8d4a5100082008200581c8adbd72dd6b5b46b27df79b1f8bcbcf0ac780d515f5b57cb08a2c9a782068200a18200581c8adbd72dd6b5b46b27df79b1f8bcbcf0ac780d515f5b57cb08a2c9a71b000000e8d4a5100082008200581c9c19c3caa333ee30c6d5d9f0ddd01ad09258a47dec0380519bcae7ac82068200a18200581c9c19c3caa333ee30c6d5d9f0ddd01ad09258a47dec0380519bcae7ac1b000000e8d4a5100082008200581c6433cd346858f15142171023c633ae0646bdc0470de5ae6f110bdf0682068200a18200581c6433cd346858f15142171023c633ae0646bdc0470de5ae6f110bdf061b000000e8d4a5100082008200581c63ec6f04e6fa18e83005a02bc57d72f7afa3a04523d016010076b40a82068200a18200581c63ec6f04e6fa18e83005a02bc57d72f7afa3a04523d016010076b40a1b000000e8d4a5100082008200581c990d9d698730cbc4c09f95be75f54e47c524c3e7ad484de626ef321482068200a18200581c990d9d698730cbc4c09f95be75f54e47c524c3e7ad484de626ef32141b000000e8d4a5100082008200581ccc8116d50326ea87caa3e46597b54e56725ff1fe39d1bc08361bc20682068200a18200581ccc8116d50326ea87caa3e46597b54e56725ff1fe39d1bc08361bc2061b000000e8d4a5100082008200581c246a121534ab486f4e47618cb192568b77a491cf4db613a80ced4d7682068200a18200581c246a121534ab486f4e47618cb192568b77a491cf4db613a80ced4d761b000000e8d4a5100082008200581c36cf17310d216fc7a2ac6122088766bbc5761129b1155d495bf8112b82068200a18200581c36cf17310d216fc7a2ac6122088766bbc5761129b1155d495bf8112b1b000000e8d4a5100082008200581c1bb104a403de68b0c03438a10d9142a595f4a9ff8162b195493bf55082068200a18200581c1bb104a403de68b0c03438a10d9142a595f4a9ff8162b195493bf5501b000000e8d4a5100082008200581cc2a47d500058e60176c437fc61be7d0cd07f0d8c871abe6d002636a182068200a18200581cc2a47d500058e60176c437fc61be7d0cd07f0d8c871abe6d002636a11b000000e8d4a5100082008200581ce8b783e08083c23c2682afcd4b7a5cb0851239e5f9c04beb2455979582068200a18200581ce8b783e08083c23c2682afcd4b7a5cb0851239e5f9c04beb245597951b000000e8d4a5100082008200581cedf156a660897651bd753aefa7af7f81d6d83dfc9bcdc4afbd36fad382068200a18200581cedf156a660897651bd753aefa7af7f81d6d83dfc9bcdc4afbd36fad31b000000e8d4a5100082008200581c30c600d4fcf006fc2067721c55fc7d3a696b93a4f382aaad75e3516682068200a18200581c30c600d4fcf006fc2067721c55fc7d3a696b93a4f382aaad75e351661b000000e8d4a5100082008200581cc7d5e024d22767a891e02834b275885e06ed04869747cff43cec91e082068200a18200581cc7d5e024d22767a891e02834b275885e06ed04869747cff43cec91e01b000000e8d4a51000ffa0f5f6"}' -H "Content-Type: application/json" | jq .certificates [ "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", "mir", { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1urancyazn5me3udh0drl9hdnrsvny6u8a4hhr7u6yufn44glu4h0r" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1ur4jyrjqc09pm6rugjyhy3psyrv05zx3zxnfnfxhk5d6f0qmnzp2k" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1urrj56p8zwx6zdqun6s72ksyh5gfdqj0p3n2pmpg94w645cwnzn0m" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uqa4yh3xrdjrfd6ljj2qf779k0h54nmgdgc6l8avaf6xsuqqk8xh8" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uz9l0wucm5qe20zh2n70f9rq424rx686l9rkye6prgudx0qw0r44z" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uzgh6wce5tyluy729h4yexc425jh4uh3sk6c44yr06qpc9c99t09s" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uqm5274d7tax9yhte2zxpcql7n5p8fy4gej39wfsadtya3c937607" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1urq70cl2j8h84yhskva0e64spfquwqum7zpkxe5f6t0p7pqvx335h" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uz9dh4ed666mg6e8maumr79uhnc2c7qd2904k47tpz3vnfcyy2em2" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uzwpns725ve7uvxx6hvlphwsrtgfyk9y0hkq8qz3n09w0tqry9ga4" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1upjr8nf5dpv0z52zzugz833n4cryd0wqgux7ttn0zy9a7ps2q548w" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1up37cmcyumap36psqkszh3tawtm6lgaqg53aq9spqpmtgzsvlws5s" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uzvsm8tfsucvh3xqn72mua04feru2fxru7k5sn0xymhny9qzgww3a" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1urxgz9k4qvnw4p7250jxt9a4fet8yhl3lcuar0qgxcduypsjh5nq9" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uqjx5ys4xj45sm6wgascevvj269h0fy3eaxmvyagpnk56asehkdm8" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uqmv79e3p5skl3az43sjyzy8v6au2as39xc32h2ft0upz2cxhp83a" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uqdmzp9yq00x3vxqxsu2zrv3g2jeta9fl7qk9vv4fyal25qx73uuh" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1urp2gl2sqpvwvqtkcsmlccd705xdqlcd3jr340ndqqnrdgguyqldu" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1ur5t0qlqszpuy0pxs2hu6jm6tjcg2y3euhuuqjlty32e09gp46x4h" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1urklz44xvzyhv5daw5awlfa007qadkpaljdum390h5m045c2gp8hm" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uqcvvqx5lncqdlpqvaepc40u05axj6un5nec924dwh34zese0v8tl" }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1urratcpy6gnk02y3uq5rfvn43p0qdmgys6t50nl58nkfrcqkq3k34" } ]

I think that instead of showing just "mir", {"certificate_type": "mir"} would be more consistent with the others, and more clear.

Also there seems to be a bit more information in the CBOR for mir, perhaps we want to show it too?

[6, [0, {[0, h'FB3C13A29D3798F1B77B47F2DDB31C19326B87ED6F71FB9A27133AD5']: 1000000000000}]]

https://github.com/input-output-hk/cardano-ledger/blob/master/eras/alonzo/test-suite/cddl-files/alonzo.cddl#L170-L176

this is tx we send during faucet startup;-) very good thing to decode IMHO. I changed to {"certificate_type": "mir"} Feel free to cut cbor if you feel so. We may also stick to it and add additional tests checking order!

piotr-iohk · 2021-12-07T12:36:17Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+                \a674a5eb1e3e568e8304581cbb114cb37d75fa05260328c235a3dae295a33d\
+                \0ba674a5eb1e3e568e1a000f42409ffff6" :: Text
+
+        let cborHexPool = fromTextEnvelope cborHex


This is how it looks like:

$ curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/transactions-decode \ > -d '{"transaction":"84a50081825820fe13857230b6db7f4d30acb043c6cd0c36595657ef79212f6b4e0cc5d5af1c8b000181825839019ae3b4936cb9e6e6e4fb854d17ed867ce10f3acdc19cdab52ab4a6de124827f09f6d5029a46b7d09854ac6a9ab16f3a991c3e2b19ac029511b000000e8d4a51000021b00038c95d0122dc003190190048482008200581c124827f09f6d5029a46b7d09854ac6a9ab16f3a991c3e2b19ac029518a03581cbb114cb37d75fa05260328c235a3dae295a33d0ba674a5eb1e3e568e5820ff097f7a12be27b1c4445b43a2d2279cc714a3c47f477a6eac3fc0ea54032ab21b000000e8d4a5100000d81e82010a581de1124827f09f6d5029a46b7d09854ac6a9ab16f3a991c3e2b19ac0295181581c124827f09f6d5029a46b7d09854ac6a9ab16f3a991c3e2b19ac0295180827824687474703a2f2f6c6f63616c686f73743a34343130372f6d657461646174612e6a736f6e5820f1941b06d889a1a9bd8a7dd72d2160aa294d81a4494f99353c6bbb120746808983028200581c124827f09f6d5029a46b7d09854ac6a9ab16f3a991c3e2b19ac02951581cbb114cb37d75fa05260328c235a3dae295a33d0ba674a5eb1e3e568e8304581cbb114cb37d75fa05260328c235a3dae295a33d0ba674a5eb1e3e568e1a000f4240a0f5f6"}' \ [ { "pool_cost": { "quantity": 0, "unit": "lovelace" }, "pool_id": "pool1hvg5evmawhaq2fsr9rprtg76u226x0gt5e62t6c78etgu2j7xtn", "pool_metadata": [ "http://localhost:44107/metadata.json", "f1941b06d889a1a9bd8a7dd72d2160aa294d81a4494f99353c6bbb1207468089" ], "pool_owners": [ "ed25519_pk1zfyz0uyld4gznfrt05yc2jkx4x43duafj8p79vv6cq54zw5zjw5" ], "pool_pledge": { "quantity": 1000000000000, "unit": "lovelace" }, "pool_margin": { "quantity": 10, "unit": "percent" } }, { "pool_id": "pool1hvg5evmawhaq2fsr9rprtg76u226x0gt5e62t6c78etgu2j7xtn", "retirement_epoch": 1000000 }, { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uqfysflsnak4q2dydd7snp22c656k9hn4xgu8c43ntqzj5g7963uw" }, { "certificate_type": "join_pool_external", "pool": "pool1hvg5evmawhaq2fsr9rprtg76u226x0gt5e62t6c78etgu2j7xtn", "reward_account": "stake_test1uqfysflsnak4q2dydd7snp22c656k9hn4xgu8c43ntqzj5g7963uw" } ]

So according to https://github.com/input-output-hk/cardano-ledger/blob/master/eras/alonzo/test-suite/cddl-files/alonzo.cddl#L154-L170 there are here:

stake_registration

pool_registration

stake_delegation

pool_retirement

In CBOR there is:

4: [[0, [0, h'124827F09F6D5029A46B7D09854AC6A9AB16F3A991C3E2B19AC02951']], [3, h'BB114CB37D75FA05260328C235A3DAE295A33D0BA674A5EB1E3E568E', h'FF097F7A12BE27B1C4445B43A2D2279CC714A3C47F477A6EAC3FC0EA54032AB2', 1000000000000, 0, 30([1, 10]), h'E1124827F09F6D5029A46B7D09854AC6A9AB16F3A991C3E2B19AC02951', [h'124827F09F6D5029A46B7D09854AC6A9AB16F3A991C3E2B19AC02951'], [], ["http://localhost:44107/metadata.json", h'F1941B06D889A1A9BD8A7DD72D2160AA294D81A4494F99353C6BBB1207468089'] ], [2, [0, h'124827F09F6D5029A46B7D09854AC6A9AB16F3A991C3E2B19AC02951'], h'BB114CB37D75FA05260328C235A3DAE295A33D0BA674A5EB1E3E568E'], [4, h'BB114CB37D75FA05260328C235A3DAE295A33D0BA674A5EB1E3E568E', 1000000]]

So 3 maps to stake_delegation -> pool_params from https://github.com/input-output-hk/cardano-ledger/blob/master/eras/alonzo/test-suite/cddl-files/alonzo.cddl:

pool_params = ( operator: pool_keyhash , vrf_keyhash: vrf_keyhash , pledge: coin , cost: coin , margin: unit_interval , reward_account: reward_account , pool_owners: set<addr_keyhash> , relays: [* relay] , pool_metadata: pool_metadata / null )

Then we have those alonzo.cddl fields mapping to the JSON keys:

operator -> pool_id

pledge -> pool_pledge

cost -> pool_cost

margin -> pool_margin

pool_owners -> pool_owners

pool_metadata -> pool_metadata

So it seems that we have those fields missing in the API:

vrf_keyhash

reward_account

relays (ok, this one is empty, but perhaps we could show [])

Also some indication what type of cert we are showing would be helpful here as well, e.g.:

[ { "certificate_type": "pool_registration_certificate", <-------------------- "pool_cost": { "quantity": 0, "unit": "lovelace" }, "pool_id": "pool1hvg5evmawhaq2fsr9rprtg76u226x0gt5e62t6c78etgu2j7xtn", "pool_metadata": [ "http://localhost:44107/metadata.json", "f1941b06d889a1a9bd8a7dd72d2160aa294d81a4494f99353c6bbb1207468089" ], "pool_owners": [ "ed25519_pk1zfyz0uyld4gznfrt05yc2jkx4x43duafj8p79vv6cq54zw5zjw5" ], "pool_pledge": { "quantity": 1000000000000, "unit": "lovelace" }, "pool_margin": { "quantity": 10, "unit": "percent" } } ]

indeed, this was intentional in a sense that I wanted to reuse ours https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/src/Cardano/Wallet/Primitive/Types.hs#L1441 And here we don't have vfr_keyhash, reward_account and relays. We may extend this type to be more aligned with ledger's one. also the same for MIR and genesis. But let's do it in separate ticket (if we really need it)

piotr-iohk · 2021-12-07T15:16:15Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+        -- tx within some wallet, so other that wallet wa, that contains
+        -- registration of reward account and joining to some pool using this
+        -- reward account
+        let serializedTxHexJoin =


This is how it looks like:

$ curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/transactions-decode \ > -d '{"transaction":"84a700818258200eaa33be8780935ca5a7c1e628a2d54402446f96236ca8f1770e07fa22ba8648060d800181825839011a2f2f103b895dbe7388acc9cc10f90dc4ada53f46c841d2ac44630789fc61d21ddfcbd4d43652bf05c40c346fa794871423b65052d7614c1b0000001748656dc8021a000237f803198d19048282008200581c89fc61d21ddfcbd4d43652bf05c40c346fa794871423b65052d7614c83028200581c89fc61d21ddfcbd4d43652bf05c40c346fa794871423b65052d7614c581cec28f33dcbe6d6400a1e5e339bd0647c0973ca6c0cf9c2bbe6838dc60e80a10082825820a922e88e8148f1bb9b9578e2640704ae8699bdd17bb9c26ed35881343c15ec485840995587f2e29c72c7ed2eb6e2381be4745503d7d2ba8de30c6cf176f82fb9074854c39ab85cb7d8e1dcdf9fb99007de2b044ba7a05ea7712b7084b4d352aa8502825820a1a07799ec226d8f2bea80dc1a4fdb25e1b2cb0dbd312a1b004b0401e901225358403ad81b75a057c419d48e1840bdeba8338206cf3df799d04328c4208b4d7a87248e604a78447c2a7acfa4488f3df5c92b01535f756e6ae6e1f23ddb9f438de10ef5f6"}' \ [ { "certificate_type": "register_reward_account_external", "reward_account": "stake_test1uzylccwjrh0uh4x5xeft7pwyps6xlfu5su2z8djs2ttkznq39723n" }, { "certificate_type": "join_pool_external", "pool": "pool1as50x0wtumtyqzs7tceeh5ry0syh8jnvpnuu9wlxswxuv48sw4w", "reward_account": "stake_test1uzylccwjrh0uh4x5xeft7pwyps6xlfu5su2z8djs2ttkznq39723n" } ]

I think this is OK, maps to the CBOR as expected:

4: [ [0, [0, h'89FC61D21DDFCBD4D43652BF05C40C346FA794871423B65052D7614C']], [2, [0, h'89FC61D21DDFCBD4D43652BF05C40C346FA794871423B65052D7614C'], h'EC28F33DCBE6D6400A1E5E339BD0647C0973CA6C0CF9C2BBE6838DC6'] ],

piotr-iohk · 2021-12-07T15:16:25Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+            ]
+
+        -- tx within other wallet than wa, containing quitting the pool
+        let serializedTxHexQuit =


LGTM:

$ curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/transactions-decode \ > -d '{"transaction":"84a700818258200eaa33be8780935ca5a7c1e628a2d54402446f96236ca8f1770e07fa22ba8648000d800181825839012c6f08b29f901657f4daf134a36a64b7b53977eb00866aadf6b8fb4d89fc61d21ddfcbd4d43652bf05c40c346fa794871423b65052d7614c1b0000001748840b48021a00021ef803198f03048182018200581c89fc61d21ddfcbd4d43652bf05c40c346fa794871423b65052d7614c0e80a10082825820a1a07799ec226d8f2bea80dc1a4fdb25e1b2cb0dbd312a1b004b0401e901225358408fec23c16be743ee592a948a4a1c9d9a4529a868d3217386985c30c6d1797c50c4928b6c3aea2825fc0677ea9550efe2270447514f1f6e189b73a0234029a802825820c15b990344122b12494a5edd1020d9eb32e34b0f82691f8e31645ddab712ff2b58402504005e990a2a6edb71e6bb1dee0c59e9328e5d698a97139567db4b7754d960ddcb738b852746c9571d7175c9263a12e40139c93929ef6ba11c1815f792b40cf5f6"}' \ > -H "Content-Type: application/json" | jq .certificates [ { "certificate_type": "quit_pool_external", "reward_account": "stake_test1uzylccwjrh0uh4x5xeft7pwyps6xlfu5su2z8djs2ttkznq39723n" } ]

CBOR:

4: [[1, [0, h'89FC61D21DDFCBD4D43652BF05C40C346FA794871423B65052D7614C']]]

paweljakubas · 2021-12-08T14:57:07Z

@piotr-iohk @Anviking I refactored json outputs - now everywhere "certificate_type" is present. Also introduced Certificate wich is a sum type, and we maintain certificates order now. Some refactoring of Shelley.Compatibility was needed (but I think it makes things clearer and simpler in the end)

Anviking

Looks reasonable enough to me.

Just noting:

If Piotr wants more tests I'd suggest some kind of property tests like forall certs. certificates $ decodeTx (addCerts (map toLedgerCert certs) emptyTx) === certs.

Anviking · 2021-12-09T19:49:40Z

lib/core/src/Cardano/Wallet/Api/Types.hs

+    => Text
+    -> a
+    -> Value
+extendAesonObject txt apipool =


A top-level extendAesonObject sounds like a very general function. But it only appends "certificate_type": txt. Could we either rename, or make it more general? It could take a Object instead, and move object ["certificate_type" .= String txt] to the call sites?

good idea, also generalized parseExtendedAesonObject

Anviking · 2021-12-09T20:27:07Z

lib/core/src/Cardano/Wallet/Api/Types.hs

+        parseDelOur obj = do
+            derPathM <-
+                (withObject "WalletDelegationCertificate" $
+                 \o -> o .:? "reward_account_path" :: Aeson.Parser (Maybe (NonEmpty (ApiT DerivationIndex)))) obj


Could we not have something more like

parseJSON = withObject "ApiAnyCertificate" $ \o -> do (ty :: String) <- o .: "certificate_type" case ty of "register_pool" -> StakePoolRegister <$> parseJSON (Object o) "join_pool" -> WalletDelegationCertificate <$> parseJSON (Object o) "quit_pool" -> WalletDelegationCertificate <$> parseJSON (Object o) ... _ -> fail $ "unknown certificate_type: " <> show ty

I think relying on case over <|> provides some more sanity when `certificate_type´ is present anyway. Even if this code is only for roundtrips and not used from the API.

add swagger - part1 clean

WalletSpec fix

paweljakubas · 2021-12-10T10:46:07Z

bors r+

iohk-bors · 2021-12-10T11:44:55Z

Build succeeded:

3047: New submit tx endpoint r=paweljakubas a=paweljakubas  adp-1171 - [x] swagger extended - [x] added impl scaffolding - [x] added submitTransaction link - [x] add first impl for submitTransaction - [x] add integration testing - [x] add impl for submitTransaction using decodeTransaction ### Comments This PR depends on #3028 in a sense that should be merged after  ### Issue Number  Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]>

paweljakubas self-assigned this Nov 19, 2021

paweljakubas force-pushed the paweljakubas/adp-1245/certs-support-in-decode-transaction branch 2 times, most recently from bcc8e17 to fb7a322 Compare November 30, 2021 13:34

paweljakubas requested review from jonathanknowles, Anviking and piotr-iohk November 30, 2021 20:21

paweljakubas marked this pull request as ready for review November 30, 2021 20:22

paweljakubas force-pushed the paweljakubas/adp-1245/certs-support-in-decode-transaction branch 3 times, most recently from e36d1d0 to 509c5b6 Compare December 2, 2021 15:23

paweljakubas mentioned this pull request Dec 2, 2021

New submit tx endpoint #3047

Merged

6 tasks

Anviking reviewed Dec 3, 2021

View reviewed changes

piotr-iohk reviewed Dec 7, 2021

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-1245/certs-support-in-decode-transaction branch from 214afbd to 61519c3 Compare December 8, 2021 14:48

paweljakubas requested review from Anviking and piotr-iohk December 8, 2021 14:57

paweljakubas force-pushed the paweljakubas/adp-1245/certs-support-in-decode-transaction branch from d832391 to fd07705 Compare December 8, 2021 15:07

Anviking approved these changes Dec 9, 2021

View reviewed changes

paweljakubas added 9 commits December 10, 2021 11:45

introduce NonWalletCertificate

1cfcbc2

adjust decodeTx in shelley and other things accordingly

1588278

adjust testing for decodeTx change

e9e5cc5

add swagger - part1

f26a72b

add swagger - part1 clean

add swagger - part2

4a90306

add swagger - part3

ca954bf

add swagger - part4

93b8147

Extend ApiDecodedTransaction and introduce ApiAnyCertificate

0a5a413

fix shelley core unit tests

40d656b

paweljakubas added 15 commits December 10, 2021 11:45

add Arbitrary for ApiDecodedTrasnaction

04a13e6

add ToJSON for ApiDecodedTrasnaction

8cb4534

add FromJSON and regenerate ApiDecodedTransaction

b42bed2

deal with other certificates on server level

30c1d29

deal with pool certificates on server level

790df3a

deal with delegation certificates on server level

b203f5c

add integration test pool joining

b729f83

add test for quit pool plus some cleanup

e1f7e5b

add integration tests for MIR

a2fbeba

add pool registration integration test

0ad28e8

add deregistration pool integration test

812200d

unify all certs json outputs

e1e3f7f

introduce sum type Certificate and refactor Shelley.Compatibility module

b844419

hlint

d75139f

WalletSpec fix

final cleaning/refactoring/improvements

2549e91

paweljakubas force-pushed the paweljakubas/adp-1245/certs-support-in-decode-transaction branch from fd07705 to 2549e91 Compare December 10, 2021 10:45

iohk-bors bot merged commit 2ca3870 into master Dec 10, 2021

iohk-bors bot deleted the paweljakubas/adp-1245/certs-support-in-decode-transaction branch December 10, 2021 11:44

WilliamKingNoel-Bot pushed a commit that referenced this pull request Dec 10, 2021

docs: Merge #3028

6a06815

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Certs support in decode transaction #3028

Certs support in decode transaction #3028

paweljakubas commented Nov 19, 2021 •

edited

Loading

Anviking left a comment

Anviking Dec 3, 2021

Anviking Dec 3, 2021

paweljakubas Dec 8, 2021

Anviking Dec 3, 2021

paweljakubas Dec 8, 2021

Anviking Dec 9, 2021

Anviking Dec 3, 2021

paweljakubas Dec 8, 2021

piotr-iohk Dec 7, 2021

piotr-iohk Dec 7, 2021

paweljakubas Dec 8, 2021

piotr-iohk Dec 7, 2021

paweljakubas Dec 8, 2021

piotr-iohk Dec 7, 2021

piotr-iohk Dec 7, 2021

paweljakubas commented Dec 8, 2021

Anviking left a comment

Anviking Dec 9, 2021

paweljakubas Dec 10, 2021

Anviking Dec 9, 2021

paweljakubas commented Dec 10, 2021

iohk-bors bot commented Dec 10, 2021

Certs support in decode transaction #3028

Certs support in decode transaction #3028

Conversation

paweljakubas commented Nov 19, 2021 • edited Loading

Comments

Issue Number

Anviking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Dec 8, 2021

Anviking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Dec 10, 2021

iohk-bors bot commented Dec 10, 2021

paweljakubas commented Nov 19, 2021 •

edited

Loading