Skip to content

Reuse TBTC from early closed auctions#43

Merged
pdyraga merged 22 commits intomainfrom
reuse-early-closed
Jun 1, 2021
Merged

Reuse TBTC from early closed auctions#43
pdyraga merged 22 commits intomainfrom
reuse-early-closed

Conversation

@lukasz-zimnoch
Copy link
Copy Markdown
Member

@lukasz-zimnoch lukasz-zimnoch commented May 7, 2021

Depends on: #39

tBTC from early closed auctions should be reused if possible. This pull request provides such functionality by introducing a tBTC surplus mechanism in the RiskManagerV1 contract. Once an auction is early closed, its transferred amount is added to the surplus pool. When a new auction is created, the underlying deposit lot size is checked against the current value of the tBTC surplus pool. In case the surplus is big enough to cover the entire deposit lot size, the deposit is liquidated directly, without performing an auction.

This change introduces a TBTC surplus mechanism
which allows to reuse tokens from multiple
early closed auctions in new auctions. It also
handles the case when subsidized auction
early closes as well and when the surplus is
big enough to liquidate a deposit without
performing an auction.
@lukasz-zimnoch lukasz-zimnoch requested a review from a team May 7, 2021 13:36
@lukasz-zimnoch
Copy link
Copy Markdown
Member Author

Ready for review. Will undraft once #39 lands on main.

@pdyraga
Copy link
Copy Markdown
Member

pdyraga commented May 19, 2021

After giving it more thought, I think this mechanism - although logically sound - is risky, a little too complex (read: expensive), and we could simplify it. My two concerns are:

  • We need another mapping, we need to update and calculate reservations. It is relatively complex and it is harder to grasp what is happening, plus, the execution costs more gas.
  • There are corner cases when, for example, the surplus is 0.9999 TBTC and we would open an auction for 0.0001 TBTC. This is less than the minimum tBTC lot size, and the governance can not predict all those cases, thus, the auction length would have to be somehow adjusted which makes the mechanism even harder.

I would consider changing this mechanism so that it keeps bookkeeping the surplus but instead of reducing the auction value, it would go straight to purchasing signer bonds if the surplus value allows for it. For example:

  • liquidation of 1 TBTC deposit, auction opened for 1 TBTC and early closed, 0.3 TBTC goes to the auctioneer.
  • liquidation of 1 TBTC deposit, auction opened for 1 TBTC and early closed, 0.8 TBTC goes to the auctioneer.
  • liquidation of 1 TBTC deposit, there is 1.1 TBTC in the surplus, instead of opening an auction, we purchase signer bonds and reduce the surplus to 0.1 TBTC.

Please give it some thought, there is a chance some gremlins are hidden in this approach.

The mechanism previously proposed has two flaws.
First, it is hard to implement and requires a lot
of additional bookkeeping in the contract
given that multiple auctions can be open at the
same time and all of them can generate a surplus.
Secondly, it might happen that the auction
gets opened for 0.0001 TBTC because the rest was
filled by the surplus. In this scenario, the
auction value is much lower than the minimum tBTC
lot size and the auction length set by the
governance could make not sense.
@lukasz-zimnoch
Copy link
Copy Markdown
Member Author

After giving it more thought, I think this mechanism - although logically sound - is risky, a little too complex (read: expensive), and we could simplify it. My two concerns are:

* We need another mapping, we need to update and calculate reservations. It is relatively complex and it is harder to grasp what is happening, plus, the execution costs more gas.

* There are corner cases when, for example, the surplus is 0.9999 TBTC and we would open an auction for 0.0001 TBTC. This is less than the minimum tBTC lot size, and the governance can not predict all those cases, thus, the auction length would have to be somehow adjusted which makes the mechanism even harder.

I would consider changing this mechanism so that it keeps bookkeeping the surplus but instead of reducing the auction value, it would go straight to purchasing signer bonds if the surplus value allows for it. For example:

* liquidation of 1 TBTC deposit, auction opened for 1 TBTC and early closed, 0.3 TBTC goes to the auctioneer.

* liquidation of 1 TBTC deposit, auction opened for 1 TBTC and early closed, 0.8 TBTC goes to the auctioneer.

* liquidation of 1 TBTC deposit, there is 1.1 TBTC in the surplus, instead of opening an auction, we purchase signer bonds and reduce the surplus to 0.1 TBTC.

Please give it some thought, there is a chance some gremlins are hidden in this approach.

You're right. I had the same feelings while implementing that approach. Please see 259f7e8. I made some change towards a simpler approach.

Base automatically changed from signer-bonds-escrow to main May 19, 2021 14:59
@lukasz-zimnoch lukasz-zimnoch marked this pull request as ready for review May 19, 2021 15:01
Comment thread contracts/RiskManagerV1.sol Outdated
Comment thread contracts/Auction.sol Outdated
Comment thread contracts/Auction.sol Outdated
Comment thread test/RiskManagerV1.test.js Outdated
Comment thread test/RiskManagerV1.test.js Outdated
# Conflicts:
#	test/RiskManagerV1.test.js
Upon early close, auction's transferred amount
should be fetched by the Auctioneer itself and
returned to the calling code. This is a better
place to do so instead of forcing the caller
to call `amountTransferred` in the right moment,
before early closing the auction.
Transferred amount is relevant only in the
moment of auction's early close. There is no
need to expose a dedicated view method. If so,
we can simplify the code and return the
transferred amount directly from `earlyClose`
method.
@lukasz-zimnoch
Copy link
Copy Markdown
Member Author

@pdyraga, all comments addressed!

Comment thread contracts/Auction.sol Outdated
Comment thread contracts/Auctioneer.sol Outdated

it("should consume a reasonable amount of gas", async () => {
await expect(parseInt(tx.gasLimit)).to.be.lessThan(480000)
await expect(parseInt(tx.gasLimit)).to.be.lessThan(485000)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this PR but we quickly need to figure out why such high gas limit is needed. Estimations in earlier PRs about purchasing signer bonds were saying about ~250k.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I will try to figure out this separately.

Comment thread test/RiskManagerV1.test.js Outdated
# Conflicts:
#	contracts/RiskManagerV1.sol
@pdyraga pdyraga merged commit 512c726 into main Jun 1, 2021
@pdyraga pdyraga deleted the reuse-early-closed branch June 1, 2021 07:15
@pdyraga pdyraga added this to the solidity/v1.0.0 milestone Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants