Remove TBTC minting from Vending Machine Deposit qualification #377

NicholasDotSol · 2019-12-03T05:04:41Z

Removing the signerFee minting from Vending also removes VendingMachine call restriction on qualifyDeposit. Instead the qualification can be done directly via DepositFunding #provideBTCFundingProof. To compensate Signer fees should be awarded on redemption.

qualifyDeposit renamed to qualifyDepositTbtcWrapper and acts as a wrapper to qualify a deposit and mint TBTC from DOT in one call.

ref: #376

We don't need ACL on DepositFunding after removing minting from proof function.

qualifyDeposit -> qualifyDepositTbtcWrapper Function is now a unqualified deposit -> TBTC wrapper dotToTbtc now mints full lot size tbtcToDot now burns full lot size Signer fees are provided during redemption.

qualifyDepositTbtcWrapper depends on dotToTbtc. Test dotToTbtc first

liamzebedee

👍

implementation/contracts/system/VendingMachine.sol

implementation/test/VendingMachineTest.js

liamzebedee · 2019-12-03T08:19:27Z

implementation/contracts/system/VendingMachine.sol

-    function qualifyDeposit(
+    /// @notice Qualifies a deposit and mints TBTC.
+    /// @dev User must allow VendingManchine to transfer DOT
+    function qualifyDepositTbtcWrapper(


Hmm - if we've removed both the qualification check and TBTC minting from this function, wouldn't it be much simpler to just call the vending machine from Deposit?

wouldn't it be much simpler to just call the vending machine from Deposit?

I don't see how this would be simpler. We would still need a call to both contracts.
Since this wrapper uses a vending machine method to mint TBTC I think it belong in Vending Machine just fine.

I can see the argument for adding the function in Deposit though. It would also be marginally cheeper since large params aren't passed around.

@Shadowfiend any thoughts here?

I don't see how this would be simpler. We would still need a call to both contracts.

I can see the argument for adding the function in Deposit though.

It depends, I'm still on the fence - where is best to validate qualification? The vending machine right now fulfills a very simple purpose - swapping between fungible (TBTC) and non-fungible (DOT) forms of tBTC. We could add some additional wrapper methods, but we're already validating qualification in the deposit now as of #377.

I like giving the vending machine a unqualifiedDotToTbtc (my name for this function). I could likewise see it on the deposit, but from a caller's perspective it preserves the clear “only calls to the vending machine will mint TBTC” relationship, which I think is a strong relationship to keep clear.

To which end, I think the name of this function isn't particularly reflective of what it's doing :) I like unqualifiedDotToTbtc for its symmetry with dotToTbtc, or unqualifiedDepositToTbtc if we want to operate on the address instead of the DOT, but am open to other ideas.

I'm terrible, sorry, one more note---I do think this function and any other wrapper functions should be grouped together at the end of the contract, unless that has unexpected consequences. We should also explicitly mark them as convenience functions/wrappers.

Ok so to recap:

qualifyDepositTbtcWrapper -> unqualifiedDepositToTbtc 👍 👍

keep the wrapper in Vending machine for context clarity

move & group related wrappers to end & label. (won't have unexpected issues here)

I like the name, unqualifiedDepositToTbtc, for now. My only gripe with it, is that qualification is only defined for funding now. We still need to be qualifying the redemption process - when we eventually approach this, what would be a name that makes sense? qualifiedDepositToBtc, redeemQualifiedDeposit? 🤔

Conceptually, I'm not sure it's the best model to think of the deposit as qualified, since we qualify it in two contexts: once during funding, and once during redemption. Qualification is something we do to ensure the integrity of the cross-chain transfers from miner attacks / reorgs, which includes TBTC -> BTC. It's the cross-chain proof which is being qualified, in funding and in redemption, rather the deposit itself.

These points are non-blocking to merging this, I just think it's easier to bring up in this context. 👍 Qualification might seem more like an aspect of the funding/redemption proofs, rather than the deposit itself. With this in mind, I had a go during the first review cycle of modelling this intuition into code (#378).

I'd like to point out the difference between a qualified deposit and qualifying a call.
A change of terminology might help avoid some confusion

Redemption already requires proof, that's already captured both in the spec, the state machine diagram, and the code we have written. Managing dynamic proof thresholds isn't currently on the table for v1 for redemption or deposit funding.

For our purposes, I think we should consider a deposit qualified when the designated minimum proof has been provided of funding. Currently that minimum is 6 blocks of work. In the future, that minimum could be dynamic or not. I don't think we'll refer to a similar such process in redemption as “qualification”---let's instead consider qualification a property of deposits that refers to how confident we are that their backing funds are available. In that sense, redemption proof should probably flip a deposit back to unqualified, as a redeemed deposit has no confidence that its backing funds are available (since they aren't 😁).

With all that said, right now we're referring to this state as “active”. Open question for me if we want to stick to that term, frankly. I'm down either way, but “qualified” kind of feels more specific. Let's not worry about that for this PR, either way.

Shadowfiend · 2019-12-04T21:29:36Z

implementation/test/VendingMachineTest.js

-  // For lack of a better design, this is the amount of TBTC exchanged for DOT's.
-  const depositValueLessSignerFee = '995000000000000000'
+  // this is the amount of TBTC exchanged for a DOT.
+  const depositValue = '1000000000000000000'


Non-blocking: is this something we could read from the contract? This is the equivalent of the lot size, yes?

Yeah we should be able to.

implementation/test/VendingMachineTest.js

Shadowfiend · 2019-12-04T21:31:53Z

implementation/test/VendingMachineTest.js

+
+      await vendingMachine.qualifyDepositTbtcWrapper(testInstance.address, _version, _txInputVector, _txOutputVector, _txLocktime, _fundingOutputIndex, _merkleProof, _txIndexInBlock, _bitcoinHeaders)
+
+      await assertBalance.tbtc(accounts[0], depositValue)


Not blocking: let's remember we need to update this once we're escrowing the signing fees.

And… Probably we should have a test once we're doing that to assert that we've escrowed said signing fees, as well.

Not blocking: let's remember we need to update this once we're escrowing the signing fees.

This was only updated because of the requirement lift on fee minting during qualification. PR diff should show signer fees subtracted. Might need to scrap this PR

The deposit shouldn't be minting any fees during qualification; the vending machine should be escrowing part of the minted TBTC for fees, assigned to the deposit for later distribution, during the regular DOT-to-TBTC flow.

That is, DOT-to-TBTC should be:

DOT -> Vending Machine Vending Machine -> <lot size> - <signing fees> TBTC -> previous DOT owner Vending Machine -> <signing fees> TBTC -> deposit fee escrow

This is the only case in which we should be escrowing right now.

In text:

Proving a deposit's funding is a completely separate process from vending TBTC. The vending machine has a convenience function whose sole purpose is to both pass a proof on to the deposit and vend TBTC. Vending TBTC mints <lot size> TBTC for the deposit and sets aside <signing fee> to the deposit for escrow, sending the remainder of the TBTC to the caller who sent the DOT in to the vending machine.

Shadowfiend · 2019-12-04T21:32:41Z

Left a couple of notes/questions.

NicholasDotSol · 2019-12-05T03:14:54Z

@Shadowfiend

building on flowdock thread:

If we need to mint TBTC again, minter authority must be considered.

We can allow provideBTCFundingProof to be callable per deposit, but we would need minter authority for every deposit in order to mint the signer fee.
The old solution was to rout the call via vending machine & mint TBTC in vending machine. The downside is that deposit Qualification calls are restricted to vendingMachine only. This might not be too bad.
provideBTCFundingProof calls to Vending Machine, Vending Machine executes a minting helper. This doesn't seem very clean.

liamzebedee · 2019-12-05T06:13:36Z

@NicholasDotSol I seem to remember we used TBTCSystem.mintTbtc before for Deposit - would that be applicable again here? Just a thought.

burn (lotsize - signer fee) on tbtcToDot

qualifyDepositTbtcWrapper -> unqualifiedDepositToTbtc get DepositValue dynamically add check for singer fee minting

NicholasDotSol · 2019-12-06T13:59:15Z

Ready on this @liamzebedee

Shadowfiend

One blocker here.

Shadowfiend · 2019-12-06T16:07:22Z

implementation/contracts/system/VendingMachine.sol

-        require(tbtcToken.balanceOf(msg.sender) >= getDepositValueLessSignerFee(), "Not enough TBTC for DOT exchange");
-        tbtcToken.burnFrom(msg.sender, getDepositValueLessSignerFee());
+        uint256 depositValueLessSignerFee = getDepositValueLessSignerFee();
+        require(tbtcToken.balanceOf(msg.sender) >= depositValueLessSignerFee, "Not enough TBTC for DOT exchange");


This is wrong. We need to burn the lot size in TBTC, not lot size minus signer fee. That's how we maintain the peg---mint 1 TBTC for 1 DOT, burn 1 TBTC for 1 DOT.

hmm. So we're not withholding the singerFee from the minted TBTC during dotToTbtc?

Whether we are or not, one DOT costs 1 TBTC, or we risk breaking the peg. Remember the peg guarantees at least 1 BTC held for each TBTC. If we ever return a DOT without burning 1 TBTC, we've potentially got a problem. The escrow changes whether the minter gets the full TBTC, but it does not change that the DOT is only worth 1 TBTC---only how that 1 TBTC is split.

ahhh. Missed this due to a bad assumption that everything equalizes during redemption / (and equalized due to burn during liquidation).

I think we need to change some vending machine design, or add a conditional. the issue i'm seeing is the following:

Whether we are or not, one DOT costs 1 TBTC

as a user I would expect to be able to call dotToTbtc <> tbtcToDot as many times as i want without losing a chunk of my TBTC every time.
if signer fee is held in dotToTbtc every time, it becomes a very expensive function.

Let’s punt signer fee to later once we’ve figured out the whole mess, but short version: fee should never be escrowed more than once. The burn here won’t change no matter what we do with signer fees: it will always equal the lot size.

or add a conditional

i.e: dotToTbtc only escrows signer fee if backing deposit has not received a signer fee.
Only first time swaps will escrow fee. I think it's worth the headache, especially since
distributeSignerFee only approves signerFee. So any extra TBTC is effectively locked forever with this design.

as an implementation note: for security reasons check would have to be _tbtc.balanceOf(backing deposit) > SignerFee, not a zero balance check.

Yep, if we stick with escrowing, it should work exactly as you describe.

Shadowfiend · 2019-12-06T16:08:14Z

implementation/contracts/system/VendingMachine.sol

+    // TODO temporary helper function
+    /// @notice Gets the Deposit lot size
+    /// @return amount in TBTC
+    function getDepositValueLessSignerFee() internal returns (uint) {


Any reason we wouldn't just… Do this inline?

We could, think this looks cleaner though. Expecting to use one of these functions in redemption wrappers

Hmmm… Whether or not we escrow signing fees, we're unlikely to be needing the lot size minus signer fee except at mint-time. Everything else is done in terms of lot size. But we can wait and see.

we're unlikely to be needing the lot size minus signer fee except at mint-time

agree

But we can wait and see.

👍

also only escrow signer fee if not already escrowed

Update tbtcToDot burn value requirement Add test for escrow mint condition on dotToTbtc

Shadowfiend · 2019-12-06T21:40:44Z

implementation/contracts/system/VendingMachine.sol

+        // If the backing Deposit does not have a signer fee in escrow, mint it. 
+        if(tbtcToken.balanceOf(address(_dotId)) < DepositUtils.signerFee()){
+            tbtcToken.mint(msg.sender, getDepositValueLessSignerFee());
+            tbtcToken.mint(address(_dotId), DepositUtils.signerFee());


This still feels wrong---in particular, that we're potentially at nonzero escrow but are minting the full signer fee to the deposit, putting us at > signer fee escrow. I can't reason my way all the way through it right now, but I want us to work through it a little.

For now, let's roll with it.

Can't find any issues with it.

Escrow gets distributed as a final step before contract halt. Any TBTC more than signerFee left in the Deposit is just locked, and this will be user error (like sending to address(0)). There is no logical reason for more TBTC to be in the deposit. If someone wants to do that for some reason there's no sence in preventing them, they're not gaining anything so shrug.

Will take a deeper look at the state machine to make sure there are no corner cases

Shadowfiend

Okayy, let's do it.

Dismissing this as it seems like we've talked through any issues that were mentioned.

liamzebedee · 2019-12-19T07:38:44Z

implementation/contracts/system/VendingMachine.sol

@@ -63,8 +34,9 @@ contract VendingMachine {
        require(depositOwnerToken.exists(_dotId), "Deposit Owner Token does not exist");
        require(isQualified(address(_dotId)), "Deposit must be qualified");

-        require(tbtcToken.balanceOf(msg.sender) >= getDepositValueLessSignerFee(), "Not enough TBTC for DOT exchange");
-        tbtcToken.burnFrom(msg.sender, getDepositValueLessSignerFee());
+        uint256 getDepositValue = getDepositValue();


@NicholasDotSol can you fix up getDepositValue in #385?

Nicholas Evans added 5 commits December 2, 2019 21:55

Remove ACL comment

55eb406

We don't need ACL on DepositFunding after removing minting from proof function.

Update proof function and TBTC minting

1c47b9f

qualifyDeposit -> qualifyDepositTbtcWrapper Function is now a unqualified deposit -> TBTC wrapper dotToTbtc now mints full lot size tbtcToDot now burns full lot size Signer fees are provided during redemption.

Update tests for new wrapper and TBTC value

0886d7e

Reorganize tests to match dependencies

b039d70

qualifyDepositTbtcWrapper depends on dotToTbtc. Test dotToTbtc first

Remove .only from VendingMachine tests

f5f7bd3

NicholasDotSol requested review from Shadowfiend and liamzebedee December 3, 2019 05:04

liamzebedee previously requested changes Dec 3, 2019

View reviewed changes

liamzebedee mentioned this pull request Dec 3, 2019

requestRedemption using DOT, instead of burning TBTC #374

Merged

Remove old comment

6148eac

Shadowfiend reviewed Dec 4, 2019

View reviewed changes

implementation/test/VendingMachineTest.js Show resolved Hide resolved

Shadowfiend reviewed Dec 4, 2019

View reviewed changes

Nicholas Evans added 5 commits December 5, 2019 11:46

Merge branch 'master' into remove-proof-minting

d98d4a0

mint signer fee on dotToTbtc exchange

899cfab

burn (lotsize - signer fee) on tbtcToDot

qualifyDepositTbtcWrapper -> unqualifiedDepositToTbtc

8171ba8

Group funcinality and separate context

1b08301

Update tests

7e0602c

qualifyDepositTbtcWrapper -> unqualifiedDepositToTbtc get DepositValue dynamically add check for singer fee minting

NicholasDotSol requested review from liamzebedee and Shadowfiend December 6, 2019 13:59

Shadowfiend suggested changes Dec 6, 2019

View reviewed changes

Nicholas Evans added 3 commits December 6, 2019 12:23

Always burn full lotSize TBTC for DOT

da8d414

also only escrow signer fee if not already escrowed

Update tests

6e14515

Update tbtcToDot burn value requirement Add test for escrow mint condition on dotToTbtc

Merge branch 'master' into remove-proof-minting

fb589b3

Shadowfiend reviewed Dec 6, 2019

View reviewed changes

Shadowfiend approved these changes Dec 6, 2019

View reviewed changes

Shadowfiend merged commit 6ee1669 into master Dec 6, 2019

Shadowfiend deleted the remove-proof-minting branch December 6, 2019 21:56

liamzebedee reviewed Dec 19, 2019

View reviewed changes


		await vendingMachine.qualifyDepositTbtcWrapper(testInstance.address, _version, _txInputVector, _txOutputVector, _txLocktime, _fundingOutputIndex, _merkleProof, _txIndexInBlock, _bitcoinHeaders)

		await assertBalance.tbtc(accounts[0], depositValue)

Remove TBTC minting from Vending Machine Deposit qualification #377

Remove TBTC minting from Vending Machine Deposit qualification #377

Conversation

NicholasDotSol commented Dec 3, 2019 • edited Loading

liamzebedee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamzebedee Dec 4, 2019 • 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

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

Shadowfiend Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

Shadowfiend commented Dec 4, 2019

NicholasDotSol commented Dec 5, 2019

liamzebedee commented Dec 5, 2019 • edited Loading

NicholasDotSol commented Dec 6, 2019

Shadowfiend 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

Shadowfiend Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicholasDotSol commented Dec 3, 2019 •

edited

Loading

liamzebedee Dec 4, 2019 •

edited

Loading

Shadowfiend Dec 5, 2019 •

edited

Loading

liamzebedee commented Dec 5, 2019 •

edited

Loading

Shadowfiend Dec 6, 2019 •

edited

Loading