finance: Adapt to new Vault #135

bingen · 2018-03-15T22:32:49Z

Besides, rename IConnector interface to IVaultConnector

izqui

Amazing work Bingen! 💰🏆

izqui · 2018-03-16T06:04:58Z

apps/finance/contracts/Finance.sol

@@ -105,23 +102,20 @@ contract Finance is AragonApp, ERC677Receiver {
     * @notice Allows to send ETH from this contract to Vault, to avoid locking them in contract forever.
     */
    function escapeHatch() public payable {
-        // convert ETH to EtherToken
-        etherToken.wrapAndCall.value(this.balance)(address(this), "Adding Funds");
+        vault.deposit.value(msg.value)(address(0), msg.sender, 0, new bytes(0));


Better to use an ETH constant than passing around address(0) everywhere

izqui · 2018-03-16T06:06:02Z

apps/finance/contracts/Finance.sol

@@ -105,23 +102,20 @@ contract Finance is AragonApp, ERC677Receiver {
     * @notice Allows to send ETH from this contract to Vault, to avoid locking them in contract forever.
     */
    function escapeHatch() public payable {
-        // convert ETH to EtherToken
-        etherToken.wrapAndCall.value(this.balance)(address(this), "Adding Funds");
+        vault.deposit.value(msg.value)(address(0), msg.sender, 0, new bytes(0));


Value must be msg.value, this code should only work when msg.value == 0

https://github.com/aragon/aragon-apps/pull/129/files#diff-9ebb8e9a2c4b877825d0aeece0fb63feR14

izqui · 2018-03-16T06:08:15Z

apps/finance/contracts/Finance.sol

        _recordIncomingTransaction(
            _token,
            msg.sender,
            _amount,
            _reference
        );
-        require(_token.transferFrom(msg.sender, address(vault), _amount));
+        vault.deposit(_token, msg.sender, _amount, new bytes(0));


So now before depositing tokens using Finance you need to create an allowance to the Vault? (otherwise there is no way this works haha). This should be made very clear in the documentation. Also pinging @onbjerg and @sohkai who are working in the frontend and this could be a blocker

Yes, this is not very user friendly, I'm changing it to get the tokens to finance app, allow to Vault and then deposit, in the way receiveApproval was done.

Hmm, I think this was always the case, to be honest. We could get away with it in one call if the token had approveAndCall / ERC677, but I guess that's not in here anymore.

Yes, but what was weird (from UX perspective) was to approve to Vault but to send to Finance. Have a look at how it is now, please, and let me know your thoughts.

izqui · 2018-03-16T06:09:01Z

apps/finance/contracts/Finance.sol

@@ -166,32 +160,35 @@ contract Finance is AragonApp, ERC677Receiver {
        external


I would remove this function entirely now that we have the new vault

izqui · 2018-03-16T06:09:47Z

apps/finance/contracts/Finance.sol

        );
-        require(token.transfer(address(vault), amount));
-        return true;
+        vault.deposit(msg.sender, _from, _amount, _userData);


Doesn't finance need to create an allowance or setOperator here for this to work?

Yes, well, as ERC777 is not part of the release and Vault doesn't fully support it yet, there was no tests for it .. so even the name and the signature are wrong! :(
Wouldn't something like this work?

ERC777(msg.sender).send(adress(vault), _amount, _userData);

Anyway, maybe I could just comment it out until we have ERC777 on Vault.

izqui · 2018-03-16T06:10:25Z

apps/finance/contracts/Finance.sol

@@ -325,12 +322,15 @@ contract Finance is AragonApp, ERC677Receiver {
        require(value > 0);


we can remove ERC20 token = ERC20(_token); above

izqui · 2018-03-16T06:13:24Z

apps/finance/test/finance.js

@@ -55,13 +60,13 @@ contract('Finance App', accounts => {
    })

    it('records ERC20 deposits', async () => {
-        await token1.approve(app.address, 5)
+        await token1.approve(vault.address, 5)


izqui · 2018-03-16T06:36:29Z

apps/finance/contracts/Finance.sol

@@ -105,23 +102,20 @@ contract Finance is AragonApp, ERC677Receiver {
     * @notice Allows to send ETH from this contract to Vault, to avoid locking them in contract forever.
     */
    function escapeHatch() public payable {


We should make this function function () payable { vault.deposit.value(this.balance)(ETH, msg.sender, this.balance, new bytes(0)) }

rather than escapeHatch

So true! 💯

izqui

looking good, thanks

sohkai

This is awesome, love how we've been able to refine this with the Vault's new API :).

A few questions / small changes noted.

sohkai · 2018-03-16T17:02:02Z

apps/finance/contracts/Finance.sol

@@ -74,8 +73,7 @@ contract Finance is AragonApp, ERC677Receiver {
        mapping (address => bool) hasBudget;
    }

-    Vault public vault;
-    EtherToken public etherToken;
+    IVaultConnector public vault;


Seems a bit odd that vault is an IVaultConnector; I haven't looked yet, but I guess Vault builds on that interface too?

Ya, it looks odd, but it's actually cool: Vault doesn't implement it, but fallback is doing delegateFwd to it. This way final functions can be called without the need of implementing them in Vault. It was @izqui 's suggestion, so pinging him here.

yep, lets do this for now.

sohkai · 2018-03-16T17:08:19Z

apps/finance/contracts/Finance.sol

-        // convert ETH to EtherToken
-        etherToken.wrapAndCall.value(this.balance)(address(this), "Adding Funds");
+    function () public payable {
+        vault.deposit.value(this.balance)(ETH, msg.sender, this.balance, new bytes(0));


Somewhat confused about this—do we want this to be the fallback (rather than an explicit "escape hatch")?

Also, should it be this and not msg.sender, e.g.:

vault.deposit.value(this.balance)(ETH, this, this.balance, new bytes(0));

Pinging @izqui, who came up with this change earlier.

no, it should be this, as for the vault's context it is coming from finance, doesn't matter where it originated

sohkai · 2018-03-16T17:09:22Z

apps/finance/contracts/Finance.sol

+        ERC20(_token).approve(address(vault), _amount);
+        // finally we can deposit them
+        vault.deposit(_token, this, _amount, new bytes(0));
+        //vault.deposit(_token, msg.sender, _amount, new bytes(0));


Should this commented out line be removed?

sure, thanks!

sohkai · 2018-03-16T17:11:26Z

apps/finance/contracts/Finance.sol

@@ -429,7 +409,7 @@ contract Finance is AragonApp, ERC677Receiver {
        return settings.periodDuration;
    }

-    function getBudget(address _token) transitionsPeriod public returns (uint256 budget, bool hasBudget, uint256 remainingBudget) {
+    function getBudget(address _token) transitionsPeriod public view returns (uint256 budget, bool hasBudget, uint256 remainingBudget) {


This can't be view; transitionsPeriod potentially changes state.

Oh, yes, thanks! I changed because I was having problems with some test, but reverting it now.

sohkai · 2018-03-16T17:13:07Z

apps/finance/test/finance.js

@@ -2,6 +2,9 @@ const { assertRevert, assertInvalidOpcode } = require('@aragon/test-helpers/asse
 const getBalance = require('@aragon/test-helpers/balance')(web3)

 const Vault = artifacts.require('Vault')
+const ETHConnector = artifacts.require('ETHConnector')
+const ERC20Connector = artifacts.require('ERC20Connector')
+//const IVaultConnector = artifacts.require('IVaultConnector')


Should we remove this?

sohkai · 2018-03-16T17:13:42Z

apps/finance/test/finance.js

@@ -13,10 +16,13 @@ contract('Finance App', accounts => {
    const periodDuration = 100
    const withdrawAddr = '0x0000000000000000000000000000000000001234'

+    const ETH='0x0'


Can we get this from Finance as Finance.ETH() (if we made that public)?

I tried, but it doesn't work for me from Truffle. I get:

TypeError: Finance.ETH is not a function

izqui · 2018-03-16T19:42:13Z

I consider Brett's comments addressed. Merging to my branch

* finance: Adapt to new Vault * finance: Adapt to new Vault (address PR comments) * finance: Adapt to new Vault (address PR comments)

finance: Adapt to new Vault

0bca179

bingen requested a review from izqui March 15, 2018 22:32

izqui reviewed Mar 16, 2018

View reviewed changes

finance: Adapt to new Vault (address PR comments)

5d552cb

izqui approved these changes Mar 16, 2018

View reviewed changes

izqui mentioned this pull request Mar 16, 2018

New Vault: ETH native support and arbitrary connectors for managing different token standards #129

Merged

5 tasks

sohkai suggested changes Mar 16, 2018

View reviewed changes

finance: Adapt to new Vault (address PR comments)

e49a9ec

bingen mentioned this pull request Mar 16, 2018

templates: adapt to new Vault #140

Merged

izqui merged commit 191d312 into good-vault Mar 16, 2018

izqui deleted the good-vault-finance-2 branch March 16, 2018 19:42

ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021

finance: Adapt to new Vault (aragon#135)

fec4997

* finance: Adapt to new Vault * finance: Adapt to new Vault (address PR comments) * finance: Adapt to new Vault (address PR comments)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

finance: Adapt to new Vault #135

finance: Adapt to new Vault #135

bingen commented Mar 15, 2018

izqui left a comment

izqui Mar 16, 2018

bingen Mar 16, 2018

izqui Mar 16, 2018

izqui Mar 16, 2018

bingen Mar 16, 2018

sohkai Mar 16, 2018

bingen Mar 16, 2018

izqui Mar 16, 2018

bingen Mar 16, 2018

izqui Mar 16, 2018

bingen Mar 16, 2018

izqui Mar 16, 2018

izqui Mar 16, 2018

izqui Mar 16, 2018

izqui Mar 16, 2018

bingen Mar 16, 2018

izqui left a comment

sohkai left a comment

sohkai Mar 16, 2018

bingen Mar 16, 2018

izqui Mar 16, 2018

sohkai Mar 16, 2018

sohkai Mar 16, 2018

izqui Mar 16, 2018

sohkai Mar 16, 2018

bingen Mar 16, 2018

sohkai Mar 16, 2018

bingen Mar 16, 2018

sohkai Mar 16, 2018

bingen Mar 16, 2018

sohkai Mar 16, 2018

bingen Mar 16, 2018

izqui commented Mar 16, 2018

		@@ -166,32 +160,35 @@ contract Finance is AragonApp, ERC677Receiver {
		external

		@@ -325,12 +322,15 @@ contract Finance is AragonApp, ERC677Receiver {
		require(value > 0);

finance: Adapt to new Vault #135

finance: Adapt to new Vault #135

Conversation

bingen commented Mar 15, 2018

izqui 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

izqui left a comment

Choose a reason for hiding this comment

sohkai 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

izqui commented Mar 16, 2018