Nazrhom/fee escrow #297

nazrhom · 2023-05-02T10:05:14Z

Part of #267

src/HydraAuction/OnChain/FeeEscrow.hs

…uction into nazrhom/fee-escrow

uhbif19 · 2023-05-03T20:44:44Z

src/HydraAuction/Tx/TermsConfig.hs

@@ -67,8 +69,8 @@ instance ToJSON AuctionTermsDynamic
 instance FromJSON AuctionTermsDynamic

 -- | Stub for tests not checking MoveToHyda. Something not existent.
-nonExistentHeadIdStub :: CurrencySymbol
-nonExistentHeadIdStub = "DEADBEEF"
+nonExistentHeadIdStub :: HeadId


Thanks 👍

uhbif19 · 2023-05-03T21:08:19Z

test/EndToEnd/Utils.hs

@@ -46,12 +46,12 @@ config =
    , configDiffVoucherExpiry = 8
    , configDiffCleanup = 10
    , configAuctionFeePerDelegate = fromJust $ intToNatural 4_000_000
-    , configStartingBid = fromJust $ intToNatural 8_000_000
-    , configMinimumBidIncrement = fromJust $ intToNatural 8_000_000
+    , configStartingBid = fromJust $ intToNatural 15_000_000


uhbif19 · 2023-05-03T21:09:22Z

src/HydraAuction/OnChain/FeeEscrow.hs

+-- Prelude imports
+import PlutusTx.Prelude
+
+-- import Prelude (quot)


Suggested change

-- import Prelude (quot)

uhbif19 · 2023-05-03T21:14:03Z

src/HydraAuction/Tx/FeeEscrow.hs

+  feeEscrowUtxo <- scriptUtxos FeeEscrow terms
+
+  feeEscrowValue <- case UTxO.pairs feeEscrowUtxo of
+    [(_, txOut)] -> pure $ txOutValue txOut


There is a scriptSingleUtxo function for that.

I don't think it work as nicely in this case, as I want both the reference to feeEscrowUtxo and to look at its value

It returns Maybe (TxIn, TxOut CtxUTxO) which seems the same that you use.

Probably the name ..Utxo is confusing, but I still do not found how to name (TxIn, TxOut) pair better. Utxo term being overloaded is awful, and fixing better vocabluary would be cool, but I do not know how to name it better.

cc @GeorgeFlerovsky

uhbif19 · 2023-05-03T21:18:07Z

src/HydraAuction/OnChain/FeeEscrow.hs

+    adaValueOf v = valueOf v adaSymbol adaToken
+
+    expectedValuePerDelegate :: Integer
+    expectedValuePerDelegate = adaValueOf singleFeeInputValue `quotient` length (delegates terms)


This is just auctionFeePerDelegate. This cannot have a reminder, cuz Escrow ensures that calculateTotalFee terms is emitted. If it was not so amount should be checked anyway.

So no need for all this stuff.

uhbif19 · 2023-05-03T21:33:52Z

src/HydraAuction/Tx/FeeEscrow.hs

+    spread 0 xs = xs
+    spread k (x : xs) = (x + 1) : spread (k - 1) xs
+
+    distributedLovelace = spread remainder (replicate delegateNumber quotient)


This is not needed because of auctionFeePerDelegate too

Or is all this code due to #298 ? In that case please put comments for spread function and write about this motivation in spec.

If we leave this kind of logic, it should have property-based unit test matching it with on-chain check.

So I think it may be simpler to just reuse this function in on-chain too. That way there is no need to test that they match each over. And it seems like there is no use for indeterminacy of remainder spreading anyway.

While this needs unit-test or property-based test anyway.

Or is all this code due to #298 ? I

No, this is only distributing ADA to the delegates in a balanced way.

If we leave this kind of logic, it should have property-based unit test matching it with on-chain check.

So I think it may be simpler to just reuse this function in on-chain too. That way there is no need to test that they match each over. And it seems like there is no use for indeterminacy of remainder spreading anyway.

While this needs unit-test or property-based test anyway.

I don't think we really need unit tests for this helper function, its quite a trivial implementation, we already have on and offchain tests for this anyway, so we are testing it.

Also, I don't think it would be nice to use this on-chain, this gives slightly more ADA to the delegates appearing first in the list. In the on-chain we should only verify that each delegates receives >= auctionFeePerDelegate.
The off-chain has to actually consume the utxo, so in case there is more ada than auctionFeePerDelegate * length delegates it can decide how to distribute the extra ada

No, this is only distributing ADA to the delegates in a balanced way.

I mean that if we do are not concerned with #298 then this ADA separation code is not needed and just auctionFeePerDelegate can be used.

I don't think we really need unit tests for this helper function, its quite a trivial implementation

Integer division and such logic is exactly the kind of stuff where people get errors like off-by-one or something like that. Also we already got off-by-one errors in intervals helpers code.

we already have on and offchain tests for this anyway, so we are testing it.

Our tests do not test ADA amount at all and this is an accident waiting to be happen.

this gives slightly more ADA to the delegates appearing first in the list

Yes, and your specification says that this is okay. So why accept different remainder distributions then if spec says that any of them are good?

The off-chain has to actually consume the utxo, so in case there is more ada than auctionFeePerDelegate * length delegates it can decide how to distribute the extra ada

What is off-chain? Why where can be extra ADA? If it is winning lot, then this does not interfere with delegate ADA distribution, cuz bidder cannot be an delegate.

@GeorgeFlerovsky Can delegate be a bidder too?

uhbif19 · 2023-05-07T22:50:05Z

src/HydraAuction/Tx/FeeEscrow.hs

+          TxOutDatumNone
+          ReferenceScriptNone
+
+zipDistributingValue :: Integer -> [PubKeyHash] -> [(PubKeyHash, Value)]


Suggested change

zipDistributingValue :: Integer -> [PubKeyHash] -> [(PubKeyHash, Value)]

zipDistributingValue :: Lovelace -> [PubKeyHash] -> [(PubKeyHash, Value)]

uhbif19

I now noticed that now this code actually uses delegates list. But there is not way to actually get it now. It should be returned by delegate server I gues.

nazrhom · 2023-05-08T08:38:38Z

@uhbif19 this is currently hardcoded to be hydraNodeActors in https://github.com/mlabs-haskell/hydra-auction/pull/297/files#diff-845d2a2cc5430818b2ff390b4bac10a80f1ebb2ada8af17b27e08fd915b82deaR84

uhbif19 · 2023-05-10T10:34:17Z

this is currently hardcoded to be hydraNodeActors

I understand. I meant real CLI. We can separate CLI fixing for that stuff in another issue, which probably requires delegates list returining, and merge this.

uhbif19 · 2023-05-10T10:36:57Z

src/HydraAuction/Tx/FeeEscrow.hs

@@ -105,8 +104,8 @@ distributeFee terms = do
          TxOutDatumNone
          ReferenceScriptNone

-zipDistributingValue :: Integer -> [PubKeyHash] -> [(PubKeyHash, Value)]
-zipDistributingValue lovelaceAmt delegatePKHs = zip delegatePKHs (lovelaceToValue . Lovelace <$> distributedLovelace)


Lovelace seems to implement divMod, so (un)wrapping is not really necessary.

uhbif19 · 2023-05-10T10:39:37Z

src/HydraAuction/OnChain/FeeEscrow.hs

-  -- Every delegate is payed at least the expected proportion of fee
-  all receivesProportionOfFee (delegates terms)
+  -- There is one input spent from the fee escrow validator with enough ADA to pay the fees
+  adaValueOf singleFeeInputValue >= expectedFeePerDelegeate * length (delegates terms)


But if all delegates get their fee, then everything is okay by construction, no?

Add FeeEscrow validator

b449c7f

nazrhom requested a review from a team as a code owner May 2, 2023 10:05

Add FeeEscrow tests

739504c

nazrhom force-pushed the nazrhom/fee-escrow branch from cfecf29 to 739504c Compare May 2, 2023 10:07

Merge branch 'staging' into nazrhom/fee-escrow

0786796

nazrhom linked an issue May 2, 2023 that may be closed by this pull request

Implement fee escrow on-chain and tx constuction #255

Closed

2 tasks

nazrhom commented May 2, 2023

View reviewed changes

src/HydraAuction/OnChain/FeeEscrow.hs Show resolved Hide resolved

nazrhom added 2 commits May 2, 2023 15:50

Update spec

38900dd

Merge branch 'nazrhom/fee-escrow' of github.com:mlabs-haskell/hydra-a…

ffa1aac

…uction into nazrhom/fee-escrow

uhbif19 reviewed May 3, 2023

View reviewed changes

uhbif19 suggested changes May 3, 2023

View reviewed changes

uhbif19 reviewed May 3, 2023

View reviewed changes

uhbif19 self-requested a review May 3, 2023 21:40

uhbif19 approved these changes May 3, 2023

View reviewed changes

uhbif19 reviewed May 7, 2023

View reviewed changes

uhbif19 suggested changes May 7, 2023

View reviewed changes

nazrhom requested a review from uhbif19 May 8, 2023 09:15

Use auctionFeePerDelegate in FeeEscrow validator

fb6541a

uhbif19 reviewed May 10, 2023

View reviewed changes

uhbif19 approved these changes May 10, 2023

View reviewed changes

nazrhom and others added 2 commits May 16, 2023 10:21

Merge branch 'staging' into nazrhom/fee-escrow

7383f4a

Update after hydraNodeActors was removed

d066f17

nazrhom enabled auto-merge May 16, 2023 09:19

nazrhom merged commit a373e2d into staging May 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nazrhom/fee escrow #297

Nazrhom/fee escrow #297

nazrhom commented May 2, 2023 •

edited by uhbif19

Loading

uhbif19 May 3, 2023

uhbif19 May 3, 2023

uhbif19 May 3, 2023

uhbif19 May 3, 2023

nazrhom May 9, 2023

uhbif19 May 11, 2023

uhbif19 May 3, 2023

uhbif19 May 3, 2023

uhbif19 May 3, 2023

uhbif19 May 7, 2023

nazrhom May 8, 2023

uhbif19 May 10, 2023

uhbif19 May 10, 2023

uhbif19 May 7, 2023

uhbif19 left a comment

nazrhom commented May 8, 2023

uhbif19 commented May 10, 2023

uhbif19 May 10, 2023

uhbif19 May 10, 2023

	zipDistributingValue :: Integer -> [PubKeyHash] -> [(PubKeyHash, Value)]
	zipDistributingValue :: Lovelace -> [PubKeyHash] -> [(PubKeyHash, Value)]

Nazrhom/fee escrow #297

Nazrhom/fee escrow #297

Conversation

nazrhom commented May 2, 2023 • edited by uhbif19 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

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

uhbif19 left a comment

Choose a reason for hiding this comment

nazrhom commented May 8, 2023

uhbif19 commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazrhom commented May 2, 2023 •

edited by uhbif19

Loading