Use a faucet to distribute funds #227

ch1bo · 2022-02-22T07:19:16Z

❄️ Introduce well-known Actor = Faucet | Alice | Bob | Carol keys as data type

❄️ Introduce seedFromFaucet to distribute funds from the Faucet actor, either Marked (as fuel) or Normal, which queries the faucet UTXO and spends it as requested.

❄️ Update all tests to use seedFromFaucet instead of the genesis/prepare commit transactions

❄️ Update the demo to use the same style of fund distribution in seed-devnet.sh. Drop the haskell exe for seeding to only have one way to maintain.

❄️ Benchmarks are working now. Needed to keep using mkGenesisTx and distribute funds to all clients in the benchmark dataset in one transaction.

ch1bo · 2022-02-23T17:17:26Z

hydra-cardano-api/src/Hydra/Cardano/Api/Lovelace.hs

+
+-- | Directly retrieve the amount of 'Lovelace' stored in a 'TxOut'.
+txOutLovelace :: TxOut ctx era -> Lovelace
+txOutLovelace (TxOut _ v _) = txOutValueToLovelace v


Not sure about this one. Just saw that there exists selectLovelace recently. You opinions?

hydra-cluster/bench/Bench/EndToEnd.hs

github-actions · 2022-02-24T12:25:08Z

Unit Test Results

    6 files ±0   76 suites ±0 5m 56s ⏱️ -31s
200 tests ±0 198 ✔️ ±0 2 💤 ±0 0 ❌ ±0

Results for commit e327adf. ± Comparison against base commit ac72d7b.

♻️ This comment has been updated with latest results.

KtorZ · 2022-02-24T18:56:03Z

hydra-cluster/src/CardanoCluster.hs

-    let addr = buildAddress vk defaultNetworkId
-        bytes = serialiseToRawBytes addr
-     in (encodeBase16 bytes, availableInitialFunds)
-
  copyCredential parentDir file = do


Nice cleanup in this module 🙏

abailly-iohk

Mostly minor comments and suggestions to clarify naming/usage and improve readability.
I have one major concern though regarding the semantics of the genDatasetConstantUTxO which seems incorrect to me, probably because I am missing something.

abailly-iohk · 2022-02-25T06:55:10Z

demo/seed-devnet.sh

-seedCommit "bob" 500000000
-seedCommit "carol" 250000000
+seedFaucet "alice" 1000000000 # 1000 Ada to commit
+seedFaucet "alice" 100000000 true # 100 Ada marked as "fuel"


suggestion: Avoid "boolean blindness" and use some variable or string instead of true

Anything evaluating to "truthy" by bash's if would work here. Bash is not really typed, but I suppose I can make this "fuel" or so?

abailly-iohk · 2022-02-25T06:58:32Z

hydra-cluster/bench/Bench/EndToEnd.hs

+seedNetwork :: RunningNode -> Dataset -> IO ()
+seedNetwork node@(RunningNode _ nodeSocket) Dataset{fundingTransaction, clientDatasets} = do
+  -- Submit the funding transaction first
+  submit defaultNetworkId nodeSocket fundingTransaction


suggestion: remove the comment which is just paraphrasing the quite explicit line of code below

abailly-iohk · 2022-02-25T06:59:28Z

hydra-cluster/bench/Bench/EndToEnd.hs

+  submit defaultNetworkId nodeSocket fundingTransaction
+  void $ waitForTransaction defaultNetworkId nodeSocket fundingTransaction
+  -- Then, fuel all clients with some 100 ADA
+  forM_ clientDatasets $ \ClientDataset{signingKey} -> do


suggestion: rather than having a comment, how about

forM_ clientDatasets fuelClientWith100Ada where fuelClientWith100Ada = ...

abailly-iohk · 2022-02-25T07:00:46Z

hydra-cluster/bench/Bench/EndToEnd.hs

  mconcat <$> mapConcurrently (uncurry clientProcessTransactionsSequence) processors
 where
-  clientProcessTransactionsSequence (Dataset{transactionsSequence}, client) clientId = do
-    let numberOfTxs = length transactionsSequence
+  clientProcessTransactionsSequence (ClientDataset{txSequence}, client) clientId = do


question: why rename the field transactionsSequence to txSequence? Now the clientProcessTransactionsSequence feels a bit odd.

That's true. I should have renamed also the function or not rename at all. Why rename? I typed it a couple times during this change and typed it wrong > 50% of those times. The plural s just before Sequence somehow trips my typing. Also we use the abbreviation transaction -> tx pervasively in this code base, so I thought I'll shorten it up.

I will change it back if you insist!?

No, I am fine with tx but then perhaps rename the other function which is also a mouthful

abailly-iohk · 2022-02-25T07:01:35Z

hydra-cluster/src/CardanoClient.hs

@@ -76,6 +72,19 @@ queryUTxOByTxIn networkId socket inputs =
          )
   in UTxO.fromApi <$> runQuery networkId socket query

+-- | Query the whole UTxO from node. Useful for debugging, but should obviously
+-- not be used in production code.


abailly-iohk · 2022-02-25T07:18:51Z

hydra-cluster/test/Test/DirectChainSpec.hs

        let cardanoKeys = [aliceCardanoVk]
        withIOManager $ \iocp -> do
          withDirectChain (contramap (FromDirectChain "alice") tracer) magic iocp nodeSocket aliceKeys alice cardanoKeys (putMVar alicesCallback) $ \Chain{postTx} -> do
-            postSeedPayment (Testnet magic) pparams availableInitialFunds nodeSocket aliceCardanoSk 100_000_000
+            void $ seedFromFaucet (Testnet magic) node aliceCardanoVk 100_000_000 Marked


Proposal: Rather than exposing the seedFromFaucet function with an additional parameter, expose 2 different functions with explicit names, eg. createFuelUTxO and createCommitUTxO or something. Additional parameters controlling the branching of a function increases the cognitive load of the function's user or reader

I'm not sure about this. The parameter does not control the branching of this function, rather what the thing you get from the faucet will be. Similar to the amount or whom it is payed to. I admit that the naming is not good and maybe data Marked = Fuel | NotMarked would be a better name?

abailly-iohk · 2022-02-25T07:19:54Z

hydra-cluster/test/Test/EndToEndSpec.hs

-                    waitForNodesConnected tracer [n1]
+                withHydraNode tracer bobSkPath [aliceVkPath, carolVkPath] tmpDir nodeSocket 2 bobSk [aliceVk, carolVk] allNodeIds $ \n2 ->
+                  withHydraNode tracer carolSkPath [aliceVkPath, bobVkPath] tmpDir nodeSocket 3 carolSk [aliceVk, bobVk] allNodeIds $ \n3 -> do
+                    -- Funds to be used as fuel by Hydra protocol transactions


comment: This comment here strengthens my belief an explicit function would be better.

Or a better name for the Marked data constructor. In my book having more functions is not making things strictly less complex.

What do you think about this invocation and dropping the comment (as it should be clear from the names):
seedFromFaucet_ defaultNetworkId node aliceCardanoVk 100_000_000 Fuel?

I prefer less parameters, plus the _ is a haskell convention which is widespread but maybe not that explicit in the long run. And it's repeated several times across different files. But I won't make it a casus belli so will let you decide.

abailly-iohk · 2022-02-25T07:21:06Z

hydra-cluster/test/Test/GeneratorSpec.hs

-            & counterexample ("\ntransactions: " <> jsonString transactionsSequence)
-            & counterexample ("\nutxo: " <> jsonString initialUTxO)
+  forAll arbitrary $ \(Positive n) -> do
+    idempotentIOProperty $ do


discovery: Did not know about this function, looks interesting and useful.

Me neither.. before yesterday :)

abailly-iohk · 2022-02-25T07:23:20Z

hydra-cluster/test/Test/LocalClusterSpec.hs

-        void $ waitForPayment networkId socket amountToPay addr
+  (RunningCluster ClusterConfig{networkId} (node : _)) -> do
+    (vk, _) <- keysFor Alice
+    void $ seedFromFaucet networkId node vk 100_000_000 Normal


Nice improvement in the size and complexity of the function 🎉

hydra-node/src/Hydra/Chain/Direct/Wallet.hs

As the third exception to the rule

Rationale: not needing to maintain two ways to set-up the devnet for the demo is desirable and a non-docker option can be provided to users which have cardano-cli in scope

Both are not used anymore

We use the faucet now instead

This also changes the definition of 'Dataset' to include all the sub-per-client-datasets.

…lients

withCreateProcess seems not to be a graceful shutdown so the node.socket file lingers in the state directory

This way, the init does not accidentally spend some output intended to be committed later.

KtorZ · 2022-02-25T08:49:04Z

hydra-cluster/src/CardanoNode.hs

 where
  generateEnvironment = do
    refreshSystemStart cfg args
    let topology = mkTopology $ peers $ ports cfg
    Aeson.encodeFile (stateDirectory </> nodeTopologyFile args) topology

+  cleanupSocketFile =
+    whenM (doesFileExist socketFile) $
+      removeFile socketFile


Why is this now necessary 🤔 ?

KtorZ

Nice work. I've got maybe one side question: what happens if the faucet runs out of the money? Not the practical implications of it but, what sort of errors are we welcomed with? Is it made clear that we are running out of money?

Asking this because of a bad memory of several hours debugging some failing e2e tests in cardano-wallet only to figure out that we had no more funds in the faucet which was causing tests to fail.

ch1bo · 2022-02-25T12:35:37Z

Nice work. I've got maybe one side question: what happens if the faucet runs out of the money? Not the practical implications of it but, what sort of errors are we welcomed with? Is it made clear that we are running out of money?

Asking this because of a bad memory of several hours debugging some failing e2e tests in cardano-wallet only to figure out that we had no more funds in the faucet which was causing tests to fail.

Good question. Haven't tried. I fear it would block because it retry forever. 😬

ch1bo force-pushed the ch1bo/faucet-distribution branch from 61d90fb to c6592db Compare February 23, 2022 09:46

ch1bo commented Feb 23, 2022

View reviewed changes

ch1bo marked this pull request as ready for review February 23, 2022 17:21

ch1bo requested review from abailly-iohk and KtorZ February 23, 2022 17:21

ch1bo mentioned this pull request Feb 24, 2022

Identify heads #209

Merged

ch1bo force-pushed the ch1bo/faucet-distribution branch 3 times, most recently from 160add8 to 1a650f2 Compare February 24, 2022 18:12

KtorZ reviewed Feb 24, 2022

View reviewed changes

abailly-iohk reviewed Feb 25, 2022

View reviewed changes

ch1bo added 18 commits February 25, 2022 08:49

Replace postSeedPayment with seedFromFaucet in EndToEndSpec

d277ec0

Replace postSeedPayment with seedFromFaucet in DirectChainSpec

15d8772

Distribute intial funds only to Faucet

1e14ef7

Import Hydra.Cardano.Api fully

540e545

As the third exception to the rule

Fix compilation of hydra-cluster

a0b3518

Send initial funds only to faucet and use it in seed-devnet.sh

5698296

Remove seed-network executable and update instructions

b8deebd

Rationale: not needing to maintain two ways to set-up the devnet for the demo is desirable and a non-docker option can be provided to users which have cardano-cli in scope

Remove generatePaymentToCommit and postSeedPayment

f3f0351

Both are not used anymore

Remove setInitialFundsInGenesisShelley

350ae14

We use the faucet now instead

Remove unused genesis verification keys

0ef2f19

Update LocalClusterSpec to use seedFromFaucet

b656ab4

Drop initialFunds from withBFTNode and withCluster

0c7efcc

Generalize mkGenesisTx to take many recipients

93adc5a

Use mkGenesisTx to fund whole all clients

d8ddfcc

This also changes the definition of 'Dataset' to include all the sub-per-client-datasets.

Update bench to use new Dataset type accordingly

1ce9abb

Debug benchmark with some outputs

b614d5e

Wait for funding transaction to be confirmed before fuelling up the c…

ac069dd

…lients

Update TUISpec with latest withBFTNode changes

71295bb

ch1bo added 7 commits February 25, 2022 08:49

Remove debug outputs from benchmark

02a0417

Only copy credentials if not yet existing

61ea7f6

Cleanup cardano-node socket file in case

6cd9bb2

withCreateProcess seems not to be a graceful shutdown so the node.socket file lingers in the state directory

Only adjust Spend redeemers in TinyWallet

0ab0924

Add queryUTxOWhole for debugging

6b12c82

Define a getFuelUTxO in our wallet and use it for seeding the initTx

4bfcda4

This way, the init does not accidentally spend some output intended to be committed later.

Get rid of unsafePerformIO

2bfd258

ch1bo force-pushed the ch1bo/faucet-distribution branch from 1a650f2 to 2bfd258 Compare February 25, 2022 07:49

ch1bo added 3 commits February 25, 2022 08:53

Use "fuel" as marker argument in seed-devnet.sh

b449406

Use code instead of comments

27b1d7c

Introduce seedFromFaucet_ and rename Marked -> Fuel

19d8f9e

KtorZ reviewed Feb 25, 2022

View reviewed changes

KtorZ approved these changes Feb 25, 2022

View reviewed changes

abailly-iohk approved these changes Feb 25, 2022

View reviewed changes

Fix redundant import

e327adf

ch1bo merged commit 5d4582f into master Feb 25, 2022

ch1bo deleted the ch1bo/faucet-distribution branch February 25, 2022 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a faucet to distribute funds #227

Use a faucet to distribute funds #227

ch1bo commented Feb 22, 2022 •

edited

Loading

ch1bo Feb 23, 2022

github-actions bot commented Feb 24, 2022 •

edited

Loading

KtorZ Feb 24, 2022

abailly-iohk left a comment

abailly-iohk Feb 25, 2022

ch1bo Feb 25, 2022

abailly-iohk Feb 25, 2022

abailly-iohk Feb 25, 2022

abailly-iohk Feb 25, 2022

ch1bo Feb 25, 2022

abailly-iohk Feb 25, 2022

abailly-iohk Feb 25, 2022

abailly-iohk Feb 25, 2022

ch1bo Feb 25, 2022

abailly-iohk Feb 25, 2022

ch1bo Feb 25, 2022

abailly-iohk Feb 25, 2022

abailly-iohk Feb 25, 2022

ch1bo Feb 25, 2022

abailly-iohk Feb 25, 2022

KtorZ Feb 25, 2022

KtorZ left a comment

ch1bo commented Feb 25, 2022

Use a faucet to distribute funds #227

Use a faucet to distribute funds #227

Conversation

ch1bo commented Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 24, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

abailly-iohk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ left a comment

Choose a reason for hiding this comment

ch1bo commented Feb 25, 2022

ch1bo commented Feb 22, 2022 •

edited

Loading

github-actions bot commented Feb 24, 2022 •

edited

Loading