Conclude new tx workflow #3449

paweljakubas · 2022-08-18T14:32:59Z

Removing byron things
Cleaning ApiAddressDestination
Foreign withdrawals not supported hence test removed
added support in constructTx, signTx and balanceTx for producing serialized tx in either hex or base64 (this one is default)
We accept in decodeTx, signTx, balanceTx and submitTx both format and for the sake of comparison and interoperability
(with for example cardano-cli) it seems like a good idea. The consequence: in each POST we have OPTIONAL
"hex_output" field which is set to true means requesting hex-encoded output.
regenerated golden tests
added proof of operability in integration testing
updated swagger

Comments

Issue Number

adp-2083

Unisay · 2022-08-19T12:06:58Z

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

+
+instance ToJSON ApiSealedTxEncoding where
+    toJSON HexEncoded = Bool True
+    toJSON Base64Encoded = Bool False


Suggested change

toJSON Base64Encoded = Bool False

toJSON Base64Encoded = Bool False

Unisay · 2022-08-19T12:11:40Z

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

 newtype ApiSerialisedTransaction = ApiSerialisedTransaction
-    { transaction :: ApiT SealedTx
+    { transaction :: (ApiT SealedTx, ApiSealedTxEncoding)
    } deriving stock (Eq, Generic, Show)
      deriving anyclass (NFData)


data ApiSerialisedTransaction = ApiSerialisedTransaction { serialisedTxSealed :: ApiT SealedTx , serialisedTxEncoding :: ApiSealedTxEncoding } deriving stock (Eq, Generic, Show) deriving anyclass (NFData)

I find this representation more readable at call sites because of the names of the accessor functions, as opposed to meaningless fst / snd, example:
this line is less readable:

let bytes = serialisedTx $ getApiT $ fst $ (tx ^. #transaction)

than this one:

let bytes = serialisedTx $ getApiT $ serialisedTxSealed $ tx ^. #transaction

sounds reasonable - done

Unisay · 2022-08-19T12:18:59Z

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

@@ -1033,6 +1046,7 @@ data ApiValidityBound
 data ApiSignTransactionPostData = ApiSignTransactionPostData
    { transaction :: !(ApiT SealedTx)
    , passphrase :: !(ApiT (Passphrase "lenient"))
+    , hexOutput :: !(Maybe Bool)


:-)

Suggested change

, hexOutput :: !(Maybe Bool)

, hexOutput :: !(Maybe !Bool)

I believe the Just of the Maybe is another arrow if we care about this:

import Data.Maybe.Strict (StrictMaybe (..)) :set -XStrictData data Foo f = Foo { foo :: f Int } λ> let !a = foo $ Foo $ Just $ undefined λ> let !a = foo $ Foo $ SJust $ undefined *** Exception: Prelude.undefined CallStack (from HasCallStack): error, called at libraries/base/GHC/Err.hs:79:14 in base:GHC.Err undefined, called at <interactive>:76:30 in interactive:Ghci18

Actually this might be the same thing as what you said, except that hexOutput :: !(Maybe !Bool) isn't allowed?

λ> data Foo = Foo { foo :: !(Maybe (!Int)) } <interactive>:83:34: error: • Unexpected strictness annotation: !Int strictness annotation cannot appear nested inside a type • In the first argument of ‘Maybe’, namely ‘(!Int)’ In the type ‘(Maybe (!Int))’ In the definition of data constructor ‘Foo’

Yes, your example demonstrates my point. StrictMaybe is the way.
https://hackage.haskell.org/package/strict-wrapper-0.0.0.0/docs/Data-Strict-Wrapper.html is interesting lib.

Alas, I don't think that we need strict fields for our Api* data types, because these are intermediate types only used for carrying data across an interface.

If we want to control space usage, it does make sense to maintain space invariants on data types that are long-lived (such as UTxO), but that is not the case here.

Unisay · 2022-08-19T12:26:08Z

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

@@ -1033,6 +1046,7 @@ data ApiValidityBound
 data ApiSignTransactionPostData = ApiSignTransactionPostData
    { transaction :: !(ApiT SealedTx)


Btw, ApiT isn't a strict container, so !(ApiT !SealedTx) makes more sense.

As @HeinrichApfelmus explained, ApiT is declared as a newtype and those guys are strict by their nature. So my comment doesn't apply.

Unisay · 2022-08-19T12:31:29Z

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

+    parseJSON = withBool "ApiSealedTxEncoding" $ \b -> case b of
+        True -> pure HexEncoded
+        False -> pure Base64Encoded


Suggested change

parseJSON = withBool "ApiSealedTxEncoding" $ \b -> case b of

True -> pure HexEncoded

False -> pure Base64Encoded

parseJSON = withBool "ApiSealedTxEncoding" $

pure . bool Base64Encoded HexEncoded

?

I'm going to call the boolice on bool. 👮‍♂️

If you really want to get rid of the intermediate name b, I would prefer

parseJSON = withBool "ApiSealedTxEncoding" . pure $ \case True -> HexEncoded False -> Base64Encoded

The explicit True and False is useful as this is about converting a flag into a more value with more meaning attached to it, and it's not obvious which case is which.

Unisay · 2022-08-19T13:08:50Z

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

+instance ToJSON ApiSealedTxEncoding where
+    toJSON HexEncoded = Bool True
+    toJSON Base64Encoded = Bool False


This instance means that ApiSealedTxEncoding is represented as Bool in the JSON.
I'd suggest to represent it as String with 2 possible values "base16" and "base64".
OpenAPI supports enum which is convenient in case like this one.

sounds like a good idea - done

Unisay · 2022-08-19T13:10:54Z

specifications/api/swagger.yaml

+        hex_output:
+          type: boolean
+          default: false


Suggested change

hex_output:

type: boolean

default: false

encoding:

type: string

enum: ["base16", "base64"]

I would agree with making this an enum.

Unisay · 2022-08-19T13:30:21Z

lib/core-integration/src/Test/Integration/Framework/DSL.hs

@@ -2366,7 +2366,7 @@ submitTx
    -> [(HTTP.Status, Either RequestException ApiTxId) -> m ()]
    -> m ApiTxId
 submitTx ctx tx expectations = do
-    let bytes = serialisedTx $ getApiT (tx ^. #transaction)
+    let bytes = serialisedTx $ getApiT $ fst $ (tx ^. #transaction)


Suggested change

let bytes = serialisedTx $ getApiT $ fst $ (tx ^. #transaction)

let bytes = serialisedTx $ getApiT $ fst $ tx ^. #transaction

and also fst is not telling me anything :(

Unisay · 2022-08-19T13:31:17Z

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

@@ -317,7 +317,7 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
                            Just m

        let era = fromApiEra $ _mainEra ctx
-        let tx = cardanoTxIdeallyNoLaterThan era $ getApiT (signedTx ^. #transaction)
+        let tx = cardanoTxIdeallyNoLaterThan era $ getApiT (fst $ signedTx ^. #transaction)


Suggested change

let tx = cardanoTxIdeallyNoLaterThan era $ getApiT (fst $ signedTx ^. #transaction)

let tx = cardanoTxIdeallyNoLaterThan era $ getApiT $ fst $ signedTx ^. #transaction

Unisay · 2022-08-19T13:31:59Z

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

@@ -384,7 +384,7 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
                            Just m

        let era = fromApiEra $ _mainEra ctx
-        let tx = cardanoTxIdeallyNoLaterThan era $ getApiT (signedTx ^. #transaction)
+        let tx = cardanoTxIdeallyNoLaterThan era $ getApiT (fst $ signedTx ^. #transaction)


Suggested change

let tx = cardanoTxIdeallyNoLaterThan era $ getApiT (fst $ signedTx ^. #transaction)

let tx = cardanoTxIdeallyNoLaterThan era $ getApiT $ fst $ signedTx ^. #transaction

Unisay · 2022-08-19T13:34:02Z

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

@@ -2034,8 +2035,10 @@ signTransaction ctx (ApiT wid) body = do
    -- TODO: The body+witnesses seem redundant with the sealedTx already. What's
    -- the use-case for having them provided separately? In the end, the client
    -- should be able to decouple them if they need to.
-    pure $ Api.ApiSerialisedTransaction
-        { transaction = ApiT sealedTx'
+    pure $ ApiSerialisedTransaction


Suggested change

pure $ ApiSerialisedTransaction

pure ApiSerialisedTransaction

HeinrichApfelmus

Thanks Pawel! 😊 Two main points:

Complete removal of the possibility to withdraw from external wallets (maybe in a follow-up pull request?)
Result type ApiConstructTransaction — I agree with Yura's suggestion of removing the pair in favor of an additional field. Choosing a short field name would limit the impact on integration tests.

HeinrichApfelmus · 2022-08-31T15:23:01Z

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

-        , ConstructByronTransaction
-        , SignByronTransaction


HeinrichApfelmus · 2022-08-31T15:33:30Z

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

@@ -540,58 +540,6 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
            , expectField #withdrawals (`shouldSatisfy` (withdrawalWith External))
            ]

-    it "TRANS_NEW_CREATE_03b - Withdrawal from external wallet" $ \ctx -> runResourceT $ do


At the end of this pull request, there seems to be a mismatch between implementation and documentation: 🤔 The swagger excludes the external withdrawals from the endpoint, but the ApiConstructTransactionData type still uses ApiWithdrawalPostData, which means that it will happily parse withdrawals to an external wallet.

Maybe going all the way of removing support is out of scope for this pull request, but I recommend doing it in a follow-up pull request, then.

I introduced ApiSelfWithdrawalPostData to address this in the current PR and adjusted all code and testing

HeinrichApfelmus · 2022-08-31T15:44:40Z

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

 data ApiConstructTransaction (n :: NetworkDiscriminant) = ApiConstructTransaction
-    { transaction :: !(ApiT SealedTx)
+    { transaction :: !(ApiT SealedTx, ApiSealedTxEncoding)


Suggested change

{ transaction :: !(ApiT SealedTx, ApiSealedTxEncoding)

{ transaction :: ApiSerialisedTransaction

I agree with Yura's comment on ApiSerialisedTransaction here.

The many pattern matches

let (txCbor,_) = getFromResponse #transaction rTx

can be avoided in favor of extracting a subfield,

let txCbor = getFromResponse (#transaction . #sealedTx) rTx

or something like that. One could even use Quantity, but I don't want to drag that type into this.

HeinrichApfelmus · 2022-08-31T15:46:07Z

specifications/api/swagger.yaml

+        hex_output:
+          type: boolean
+          default: false


I would agree with making this an enum.

HeinrichApfelmus · 2022-08-31T15:53:58Z

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

@@ -1033,6 +1046,7 @@ data ApiValidityBound
 data ApiSignTransactionPostData = ApiSignTransactionPostData
    { transaction :: !(ApiT SealedTx)
    , passphrase :: !(ApiT (Passphrase "lenient"))
+    , hexOutput :: !(Maybe Bool)


Alas, I don't think that we need strict fields for our Api* data types, because these are intermediate types only used for carrying data across an interface.

If we want to control space usage, it does make sense to maintain space invariants on data types that are long-lived (such as UTxO), but that is not the case here.

paweljakubas · 2022-09-07T12:39:42Z

bors r+

3449: Conclude new tx workflow r=paweljakubas a=paweljakubas  - [x] Removing byron things - [x] Cleaning ApiAddressDestination - [x] Foreign withdrawals not supported hence test removed - [x] added support in constructTx, signTx and balanceTx for producing serialized tx in either hex or base64 (this one is default) We accept in decodeTx, signTx, balanceTx and submitTx both format and for the sake of comparison and interoperability (with for example cardano-cli) it seems like a good idea. The consequence: in each POST we have OPTIONAL "hex_output" field which is set to true means requesting hex-encoded output. - [x] regenerated golden tests - [x] added proof of operability in integration testing - [x] updated swagger ### Comments  ### Issue Number adp-2083  Co-authored-by: Pawel Jakubas <[email protected]>

iohk-bors · 2022-09-07T12:45:15Z

Build failed:

buildkite/cardano-wallet

#expected hlint

paweljakubas · 2022-09-07T13:09:20Z

bors r+

3449: Conclude new tx workflow r=paweljakubas a=paweljakubas  - [x] Removing byron things - [x] Cleaning ApiAddressDestination - [x] Foreign withdrawals not supported hence test removed - [x] added support in constructTx, signTx and balanceTx for producing serialized tx in either hex or base64 (this one is default) We accept in decodeTx, signTx, balanceTx and submitTx both format and for the sake of comparison and interoperability (with for example cardano-cli) it seems like a good idea. The consequence: in each POST we have OPTIONAL "hex_output" field which is set to true means requesting hex-encoded output. - [x] regenerated golden tests - [x] added proof of operability in integration testing - [x] updated swagger ### Comments  ### Issue Number adp-2083  Co-authored-by: Pawel Jakubas <[email protected]>

iohk-bors · 2022-09-07T14:06:48Z

Build failed:

ci/hydra-build:required

                 ( Object
                     ( fromList
                         [
                             ( "code"
                             , String "bad_request"
                             )
                         ,
                             ( "message"
                             , String "Error in $: parsing ApiSerialisedTransaction object failed, expected Object, but encountered String"
                             )
                         ]
                     )
                 )
             )
         )
       expected: Status {
                   statusCode = 202,
                   statusMessage = "Accepted"
                 }
        but got: Status {
                   statusCode = 400,
                   statusMessage = "Bad Request"
                 }

  To rerun use: --match "/API Specifications/NEW_SHELLEY_TRANSACTIONS/Plutus scenarios/mint-burn/"

paweljakubas · 2022-09-07T15:19:03Z

bors r+

3449: Conclude new tx workflow r=paweljakubas a=paweljakubas  - [x] Removing byron things - [x] Cleaning ApiAddressDestination - [x] Foreign withdrawals not supported hence test removed - [x] added support in constructTx, signTx and balanceTx for producing serialized tx in either hex or base64 (this one is default) We accept in decodeTx, signTx, balanceTx and submitTx both format and for the sake of comparison and interoperability (with for example cardano-cli) it seems like a good idea. The consequence: in each POST we have OPTIONAL "hex_output" field which is set to true means requesting hex-encoded output. - [x] regenerated golden tests - [x] added proof of operability in integration testing - [x] updated swagger ### Comments  ### Issue Number adp-2083  Co-authored-by: Pawel Jakubas <[email protected]>

iohk-bors · 2022-09-07T17:53:10Z

Build failed:

ci/hydra-build:required

  src/Test/Integration/Scenario/CLI/Shelley/HWWallets.hs:100:5: 
  182) CLI Specifications, SHELLEY_CLI_HW_WALLETS, HW_WALLETS_01x - Restoration from account public key preserves funds
       uncaught exception: IOException of type UserError
       user error (No more faucet wallets of type shelley! You may need to manually add entries to the relevant list in lib/core-integration/src/Test/Integration/Faucet.hs)

  To rerun use: --match "/CLI Specifications/SHELLEY_CLI_HW_WALLETS/HW_WALLETS_01x - Restoration from account public key preserves funds/"

  src/Test/Integration/Scenario/CLI/Shelley/HWWallets.hs:158:9: 
  183) CLI Specifications, SHELLEY_CLI_HW_WALLETS, HW_WALLETS_03 - Cannot do operations requiring private key, Cannot send tx
       uncaught exception: IOException of type UserError
       user error (No more faucet wallets of type shelley! You may need to manually add entries to the relevant list in lib/core-integration/src/Test/Integration/Faucet.hs)

  To rerun use: --match "/CLI Specifications/SHELLEY_CLI_HW_WALLETS/HW_WALLETS_03 - Cannot do operations requiring private key/Cannot send tx/"

  src/Test/Integration/Scenario/CLI/Shelley/HWWallets.hs:223:9: 
  184) CLI Specifications, SHELLEY_CLI_HW_WALLETS, HW_WALLETS_04 - Can manage HW wallet the same way as others, Can get tx fee
       uncaught exception: IOException of type UserError
       user error (No more faucet wallets of type shelley! You may need to manually add entries to the relevant list in lib/core-integration/src/Test/Integration/Faucet.hs)

  To rerun use: --match "/CLI Specifications/SHELLEY_CLI_HW_WALLETS/HW_WALLETS_04 - Can manage HW wallet the same way as others/Can get tx fee/"

  src/Test/Integration/Scenario/CLI/Network.hs:60:18: 
  185) CLI Specifications, COMMON_CLI_NETWORK, CLI_NETWORK - cardano-wallet network information
       uncaught exception: ErrorCall
       Maybe.fromJust: Nothing
       CallStack (from HasCallStack):
         error, called at libraries/base/Data/Maybe.hs:148:21 in base:Data.Maybe
         fromJust, called at src/Test/Integration/Scenario/CLI/Network.hs:60:18 in cardano-wallet-core-integration-2022.8.16-9Nc58BLxxeemxV55PKwgZ:Test.Integration.Scenario.CLI.Network

  To rerun use: --match "/CLI Specifications/COMMON_CLI_NETWORK/CLI_NETWORK - cardano-wallet network information/"

Randomized with seed 955355607

Finished in 8011.4222 seconds, used 563.8291 seconds of CPU time
953 examples, 185 failures, 14 pending

Interesting...

Searching for "Applied {1,0}" it seems the pool worker saw blocks, but no wallet? 🥴

#3461

paweljakubas · 2022-09-07T18:31:17Z

bors retry

iohk-bors · 2022-09-07T19:28:06Z

Build succeeded:

paweljakubas requested review from jonathanknowles, Anviking, Unisay and piotr-iohk August 18, 2022 14:32

paweljakubas self-assigned this Aug 18, 2022

Unisay reviewed Aug 19, 2022

View reviewed changes

HeinrichApfelmus approved these changes Aug 31, 2022

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-2083/txflow-conclude branch from 752b5de to c52d97c Compare September 6, 2022 17:21

paweljakubas added 12 commits September 7, 2022 13:01

remove construct and sign from Byron

a4ff91c

construct does not support external withdrawals

1eceee8

clean ApiPaymentDestination

d44b9cd

allow both base16 and base64 (here defalut) for constructTransaction

c45edb3

add integration test

58e10b0

allow return hex/base64 for signTransaction

1dc5d34

regenerate core unit tests

9267d6a

allow return hex/base64 for balanceTransaction

fddd3b4

add balanceTransaction integration test

1fec09b

update endpoints info in swagger

9c0fc07

refactor ApiSerialisedTransaction

173dadc

refactor ApiSealedTxEncoding

cab0cd3

deal with withdrawal in ApiConstructPostData

3918890

paweljakubas force-pushed the paweljakubas/adp-2083/txflow-conclude branch from c52d97c to 3918890 Compare September 7, 2022 12:38

hlint

af38057

fix plutus scenarios

88eb9f8

iohk-bors bot merged commit 583b6b3 into master Sep 7, 2022

iohk-bors bot deleted the paweljakubas/adp-2083/txflow-conclude branch September 7, 2022 19:28

WilliamKingNoel-Bot pushed a commit that referenced this pull request Sep 7, 2022

docs: Merge #3449

d0eab58

	toJSON Base64Encoded = Bool False
	toJSON Base64Encoded = Bool False

		@@ -1033,6 +1046,7 @@ data ApiValidityBound
		data ApiSignTransactionPostData = ApiSignTransactionPostData
		{ transaction :: !(ApiT SealedTx)

	let bytes = serialisedTx $ getApiT $ fst $ (tx ^. #transaction)
	let bytes = serialisedTx $ getApiT $ fst $ tx ^. #transaction

	let tx = cardanoTxIdeallyNoLaterThan era $ getApiT (fst $ signedTx ^. #transaction)
	let tx = cardanoTxIdeallyNoLaterThan era $ getApiT $ fst $ signedTx ^. #transaction

	pure $ ApiSerialisedTransaction
	pure ApiSerialisedTransaction

	{ transaction :: !(ApiT SealedTx, ApiSealedTxEncoding)
	{ transaction :: ApiSerialisedTransaction

Conclude new tx workflow #3449

Conclude new tx workflow #3449

Conversation

paweljakubas commented Aug 18, 2022

Comments

Issue Number

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

Anviking Aug 25, 2022 • edited Loading

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

HeinrichApfelmus Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Unisay Aug 19, 2022 • edited Loading

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

HeinrichApfelmus left a comment • edited Loading

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 Sep 7, 2022

iohk-bors bot commented Sep 7, 2022 • edited by Anviking Loading

paweljakubas commented Sep 7, 2022

iohk-bors bot commented Sep 7, 2022 • edited by Anviking Loading

paweljakubas commented Sep 7, 2022

iohk-bors bot commented Sep 7, 2022 • edited by Anviking Loading

paweljakubas commented Sep 7, 2022

iohk-bors bot commented Sep 7, 2022

Anviking Aug 25, 2022 •

edited

Loading

HeinrichApfelmus Aug 31, 2022 •

edited

Loading

Unisay Aug 19, 2022 •

edited

Loading

HeinrichApfelmus left a comment •

edited

Loading

iohk-bors bot commented Sep 7, 2022 •

edited by Anviking

Loading

iohk-bors bot commented Sep 7, 2022 •

edited by Anviking

Loading

iohk-bors bot commented Sep 7, 2022 •

edited by Anviking

Loading