feat(emp): add trimExcess function to send excess tokens #1975

mrice32 · 2020-09-09T22:24:37Z

Motivation

#1974.

Summary

Added a function to send excess collateral to a prespecified beneficiary address. This address is passed in by the deployer. The excess collateral is assumed to be any collateral not included in pfc. This is an EMP-specific assumption that I would appreciate feedback on (correctness and understandability). It may be clearer to make a distinct internal function called _trackedBalance() or similar that pfc calls.

Note: this still needs tests. I wanted to get feedback before putting time into tests.

Issue(s)

Fixes #1974.

…rmined address Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree

Overall this is a great solution! much more generic than the re-direct functionality. bit of a pity that it adds an extra param to the contractor which will make the test churn quite high but that's unavoidable given the desired features of this change.

chrismaree · 2020-09-10T06:37:06Z

packages/core/contracts/financial-templates/expiring-multiparty/ExpiringMultiPartyCreator.sol

@@ -38,6 +38,7 @@ contract ExpiringMultiPartyCreator is ContractCreator, Testable, Lockable {
        FixedPoint.Unsigned minSponsorTokens;
        uint256 withdrawalLiveness;
        uint256 liquidationLiveness;
+        address beneficiary;


nit: beneficiary as a variable name makes sense in the context of this PR but taken out of context it is a bit confusing. perhaps a different variable name for this?

prehaps excessTokenBeneficiary?

chrismaree · 2020-09-10T06:46:12Z

packages/core/contracts/financial-templates/expiring-multiparty/PricelessPositionManager.sol

+        if (address(token) == address(collateralCurrency)) {
+            // If it is the collateral currency, send only the amount that the contract is not tracking.
+            // Note: this could be due to rounding error or balance-chainging tokens, like aTokens.
+            token.safeTransfer(beneficiary, balance.sub(pfc()).rawValue);


This makes sense to me. PFC is simply the rawTotalPositionCollateral adjusted for fees. This is updated every time someone deposits or withdraws from the contract. If the balance shifts through some other action (an a-token or someone depositing collateral directly to the emp) then the balance of call will be higher than the pfc and this function will trip any of that excess balance off.

In theory, this could also let us recover funds that are incorrectly sent to the contract.

Here you do not perform any checks on who beneficiary is assigned to. It'll default to the 0x000 address as a result. Because this function has no access control someone could grief the EMP if it has any excess collateral by sending it to the 0x000 address. I'd consider either adding a check that enforces that the beneficiary is not the 0 address or perhaps setting it to the Store?

Good point. I think checking that it's not 0x0 in the constructor is probably the simplest solution.

Done. I added it to the validation done in the creator so the check wouldn't affect the bytecode size.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai · 2020-09-10T15:46:31Z

packages/core/contracts/financial-templates/expiring-multiparty/PricelessPositionManager.sol

+        if (address(token) == address(collateralCurrency)) {
+            // If it is the collateral currency, send only the amount that the contract is not tracking.
+            // Note: this could be due to rounding error or balance-chainging tokens, like aTokens.
+            token.safeTransfer(excessTokenBeneficiary, balance.sub(pfc()).rawValue);


So how does the balance of the contract diverge from pfc()?

user accidentally sends collateral to the contract

interest denominated in collateral accrues to contract

anything else?

I think those are the only two cases. But obviously, the viability of this change depends on that assumption. I'm going to take another pass through the contract to ensure there are no places where the balance of the contract is expected to deviate from pfc.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 · 2020-09-10T20:29:27Z

@nicholaspai @chrismaree I've added tests. PTAL.

coveralls · 2020-09-10T20:52:40Z

Coverage increased (+0.06%) to 92.754% when pulling b1a47ae on mrice32:trim_excess into 5643675 on UMAprotocol:master.

nicholaspai · 2020-09-10T20:59:14Z

packages/core/test/financial-templates/PricelessPositionManager.js

@@ -2,6 +2,7 @@
 const { PositionStatesEnum, didContractThrow } = require("@umaprotocol/common");
 const truffleAssert = require("truffle-assertions");
 const { interfaceName } = require("@umaprotocol/common");
+const { assert } = require("chai");


nit: do you need to this assert import? I thought it came with Truffle for free

Nope, my editor just got overzealous :). Nice catch!

nicholaspai

I think this is simplest implementation for this, nice! And I think the test coverage overall is great. It'd be better to run this basically after every single test in a single afterEach() hook, but absent that I think we should add the trimExcess test after:

Precision Loss tests in Liquidatable and PPM
Non-18 decimal collateral EMP's

mrice32 · 2020-09-10T21:21:42Z

I think this is simplest implementation for this, nice! And I think the test coverage overall is great. It'd be better to run this basically after every single test in a single afterEach() hook, but absent that I think we should add the trimExcess test after:

Precision Loss tests in Liquidatable and PPM

Non-18 decimal collateral EMP's

I see what you're saying. Will add!

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 · 2020-09-10T21:36:09Z

I think this is simplest implementation for this, nice! And I think the test coverage overall is great. It'd be better to run this basically after every single test in a single afterEach() hook, but absent that I think we should add the trimExcess test after:

Precision Loss tests in Liquidatable and PPM

Non-18 decimal collateral EMP's

I see what you're saying. Will add!

@nicholaspai added an afterEach for both the PPM and Liquidator tests. PTAL

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree · 2020-09-11T07:02:08Z

packages/core/scripts/DeploySyntheticTokens.js

@@ -99,7 +101,8 @@ const actualDeploy = async inputCsv => {
      disputeBondPct: percentToFixedPoint(params.disputeBond),
      sponsorDisputeRewardPct: percentToFixedPoint(params.sponsorDisputeReward),
      disputerDisputeRewardPct: percentToFixedPoint(params.disputeReward),
-      minSponsorTokens: percentToFixedPoint(params.minSponsorTokens)
+      minSponsorTokens: percentToFixedPoint(params.minSponsorTokens),
+      excessTokenBeneficiary: store.address


nice. good default.

chrismaree

looks great! effective and clean testing. overall a perfect implementation of this logic.

nicholaspai · 2020-09-11T11:48:27Z

packages/core/test/financial-templates/Liquidatable.js

+    assert.notEqual(beneficiaryCollateralBalance.toString(), "0");
+  };
+
+  afterEach(async () => {


This is awesome, does this make the rest of the expectNoExcessCollateralToTrim() calls redundant now?

Good point. Most of them are probably still necessary because we want to check that there are no intermediate steps that leave untracked tokens sitting in the contract. For instance, I was a little worried that somewhere in the liquidation and dispute flow that a bond or final fee was in the contract but not added to pfc, which would allow someone to drain it using this method. I'll go back and remove any that occur as the last step in a test case, though.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 · 2020-09-16T11:36:23Z

@nicholaspai merged PTAL.

feat(emp): add trimExcess function to send excess tokens to a predete…

20daa4c

…rmined address Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 requested review from nicholaspai and chrismaree September 9, 2020 22:24

chrismaree reviewed Sep 10, 2020

View reviewed changes

comments

2efc617

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 requested a review from chrismaree September 10, 2020 13:55

nicholaspai reviewed Sep 10, 2020

View reviewed changes

add tests

3ce5a7d

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 marked this pull request as ready for review September 10, 2020 20:29

mrice32 requested a review from nicholaspai September 10, 2020 20:29

nicholaspai reviewed Sep 10, 2020

View reviewed changes

nicholaspai approved these changes Sep 10, 2020

View reviewed changes

WIP

d1b18f6

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 requested a review from nicholaspai September 10, 2020 21:36

mrice32 added 5 commits September 10, 2020 18:46

WIP

57ede29

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

Update PricelessPositionManager.js

e81b8bf

WIP

59b7f52

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

b80606a

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

Merge remote-tracking branch 'upstream/master' into trim_excess

662c62e

chrismaree reviewed Sep 11, 2020

View reviewed changes

chrismaree approved these changes Sep 11, 2020

View reviewed changes

nicholaspai reviewed Sep 11, 2020

View reviewed changes

mrice32 added 4 commits September 11, 2020 09:00

WIP

d8ffc75

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

1efe2c7

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

9497eb9

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

bc6e302

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 added 3 commits September 14, 2020 17:36

Update PricelessPositionManager.sol

332b56e

WIP

6d192de

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

Merge branch 'master' into trim_excess

b1a47ae

mrice32 requested a review from nicholaspai September 16, 2020 11:36

mrice32 merged commit 658f4d9 into UMAprotocol:master Sep 16, 2020

mrice32 deleted the trim_excess branch September 16, 2020 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(emp): add trimExcess function to send excess tokens #1975

feat(emp): add trimExcess function to send excess tokens #1975

mrice32 commented Sep 9, 2020

chrismaree left a comment

chrismaree Sep 10, 2020

chrismaree Sep 10, 2020

mrice32 Sep 10, 2020

mrice32 Sep 10, 2020

chrismaree Sep 10, 2020

chrismaree Sep 10, 2020

mrice32 Sep 10, 2020

mrice32 Sep 10, 2020

nicholaspai Sep 10, 2020

mrice32 Sep 10, 2020

mrice32 commented Sep 10, 2020

coveralls commented Sep 10, 2020 •

edited

Loading

nicholaspai Sep 10, 2020

mrice32 Sep 10, 2020

nicholaspai left a comment

mrice32 commented Sep 10, 2020

mrice32 commented Sep 10, 2020

chrismaree Sep 11, 2020

chrismaree left a comment

nicholaspai Sep 11, 2020

mrice32 Sep 11, 2020

mrice32 commented Sep 16, 2020

feat(emp): add trimExcess function to send excess tokens #1975

feat(emp): add trimExcess function to send excess tokens #1975

Conversation

mrice32 commented Sep 9, 2020

chrismaree 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

mrice32 commented Sep 10, 2020

coveralls commented Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai left a comment

Choose a reason for hiding this comment

mrice32 commented Sep 10, 2020

mrice32 commented Sep 10, 2020

Choose a reason for hiding this comment

chrismaree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 commented Sep 16, 2020

coveralls commented Sep 10, 2020 •

edited

Loading