New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make send and transfer work across proxies #281

Merged
merged 23 commits into from May 13, 2018

Conversation

Projects
None yet
4 participants
@bingen
Copy link
Contributor

bingen commented Mar 28, 2018

Besides, add function to recover funds.

Closes #275.

@bingen bingen requested review from izqui and sohkai Mar 28, 2018

Make send and transfer work across proxies
Besides, add function to recover funds.

Closes #275.

@bingen bingen force-pushed the issue275 branch from 3b7aa32 to e1883fa Mar 28, 2018

@bingen bingen changed the title [WIP] Make send and transfer work across proxies Make send and transfer work across proxies Mar 28, 2018

@bingen bingen changed the title Make send and transfer work across proxies [WIP] Make send and transfer work across proxies Mar 28, 2018

@bingen bingen force-pushed the issue275 branch from 7b50cb5 to 78faafc Mar 29, 2018

* @param _name Name of the app
* @return ID of app
*/
function setDefaultVaultId(bytes32 _namespace, bytes32 _name) auth(APP_MANAGER_ROLE, arr(_namespace, _name)) kernelIntegrity public returns (bytes32) {

This comment has been minimized.

@izqui

izqui Mar 29, 2018

Member

We should hardcode namespace to be the APPS_NAMESPACE

This comment has been minimized.

@izqui

izqui Mar 29, 2018

Member

This will also remove the need to check kernelIntegrity

@@ -16,4 +16,5 @@ contract KernelConstants {

contract KernelStorage is KernelConstants {
mapping (bytes32 => address) public apps;
bytes32 public defaultVaultId;

This comment has been minimized.

@izqui

izqui Mar 29, 2018

Member

Why do we need this extra storage?

@bingen bingen changed the title [WIP] Make send and transfer work across proxies Make send and transfer work across proxies Mar 29, 2018

@bingen bingen force-pushed the issue275 branch from fb7350c to 0bfe25c Mar 29, 2018

bingen added some commits Mar 29, 2018

Hardcode default vault namespace
Address issue 275 PR comments.
@izqui

izqui approved these changes Apr 3, 2018

Copy link
Member

izqui left a comment

Looking good. Just this simple convenience thing and we are good to go from my side. Lets wait for brett to review too as this is pretty critical code.

* @return ID of app
*/
function setDefaultVaultId(bytes32 _name) auth(APP_MANAGER_ROLE, arr(APP_BASES_NAMESPACE, _name)) public {
defaultVaultId = keccak256(APP_BASES_NAMESPACE, _name);

This comment has been minimized.

@izqui

izqui Apr 3, 2018

Member

I think we should make this part of the initialization of the kernel. Set this to defaultVaultId = keccak256(APP_BASES_NAMESPACE, namehash('vault.aragonpm.eth')

This comment has been minimized.

@izqui

izqui Apr 3, 2018

Member

See #282, as the name should be the appId for the vault

This comment has been minimized.

@bingen

bingen Apr 3, 2018

Contributor

Ok, I'll do this once we have #282 done and merged.

This comment has been minimized.

@izqui

izqui Apr 10, 2018

Member

That should have been APP_ADDR_NAMESPACE

require(isContract(vault));

if (_token == ETH) {
vault.transfer(address(this).balance);

This comment has been minimized.

@izqui

izqui Apr 3, 2018

Member

Maybe we could make this a proper vault transfer so it gets logged. We just need to convert it to require(vault.call.value(address(this).balance)())

vault.transfer(address(this).balance);
} else {
uint256 amount = ERC20(_token).balanceOf(this);
ERC20(_token).transfer(vault, amount);

This comment has been minimized.

@izqui

izqui Apr 3, 2018

Member

Also this should be a proper deposit, need to give it an extra thought about how we can support different deposit logic to the vault depending on the token standard. I would say lets leave it like this for now, but lets give it an extra thought

Make send and transfer work across proxies
Address issue 275 PR comments.

@bingen bingen force-pushed the issue275 branch from e408a7b to 5d84063 Apr 3, 2018

bingen added some commits Apr 4, 2018

Set default vault id on kernel init
Address issue 275 PR comments.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 4, 2018

Coverage Status

Coverage increased (+0.3%) to 99.379% when pulling 1dd728e on issue275 into 8aef743 on dev.

bingen added some commits Apr 4, 2018

Fix coverage
Add tests for checkOracle and kernelIntegrity.
@izqui

This comment has been minimized.

Copy link
Member

izqui commented Apr 6, 2018

Looking good. @sohkai pls review

@sohkai
Copy link
Member

sohkai left a comment

Found an issue with how we're using APP_BASES_NAMESPACE: we should use APP_ADDR_NAMESPACE when creating the defaultVaultId instead and make sure we call setApp() with the vault in the aragon-apps templates.

/**
* @dev Set the resolving address of an app instance or base implementation
* @param _name Name of the app
* @return ID of app

This comment has been minimized.

@sohkai

sohkai Apr 9, 2018

Member

These comments should probably be changed?

*/
function getDefaultVault() public view returns (address) {
return apps[defaultVaultId];
}

This comment has been minimized.

@sohkai

sohkai Apr 9, 2018

Member

I don't think we need this since KernelProxy already implements this?

/**
* @notice Send funds to default Vault. This contract should never receive funds,
* but in case it does, this function allows to recover them.
* @param _token Token balance to be sent to Vault. ETH(0x0) for ether.

This comment has been minimized.

@sohkai

sohkai Apr 9, 2018

Member

Change to Token to be sent to Vault



contract FundsProxy is DelegateProxy, ERCProxy {
address constant public ETH = 0x0;

This comment has been minimized.

@sohkai

sohkai Apr 9, 2018

Member

I'm getting worried we're using this in all sorts of places now. Maybe we should make a EtherToken.sol that just exports this constant.

This comment has been minimized.

@izqui

izqui Apr 9, 2018

Member

also we should use address(0) everywhere as it is more descriptive.


function () payable public {
// all calls except for send or transfer
if (msg.gas > 10000) {

This comment has been minimized.

@sohkai

sohkai Apr 9, 2018

Member

We should export a constant in the DelegateProxy for this 10k expectation.

if (msg.gas > 10000) {
address target = implementation();
require(target != 0); // if app code hasn't been set yet, don't call
delegatedFwd(target, msg.data);

This comment has been minimized.

@sohkai

sohkai Apr 9, 2018

Member

This silently returns, which is kind of funky when you read it without any context.

What do you think about restructuring this so that we flip the conditionals, e.g.

if (msg.gas < 10000) {
    // Must be a send()-like call (low gas) with value
    require(msg.value > 0 && msg.data.length == 0);
    ProxyDeposit(msg.sender, msg.value);
} else {
    ...
}
// all calls except for send or transfer
if (msg.gas > 10000) {
address target = implementation();
require(target != 0); // if app code hasn't been set yet, don't call

This comment has been minimized.

@sohkai

sohkai Apr 9, 2018

Member

We can remove this check here and in AppProxyBase.

@@ -26,6 +26,8 @@ contract Kernel is IKernel, KernelStorage, Initializable, AppProxyFactory, ACLSy
_setApp(APP_ADDR_NAMESPACE, ACL_APP_ID, acl);

acl.initialize(_permissionsCreator);

defaultVaultId = keccak256(APP_BASES_NAMESPACE, apmNamehash('vault'));

This comment has been minimized.

@sohkai

sohkai Apr 9, 2018

Member

We should change this to use APP_ADDR_NAMESPACE.

@@ -79,6 +76,17 @@ contract TestDelegateProxy is DelegateProxy {
Assert.isFalse(result, "should return false");
}

/* TODO

This comment has been minimized.

@sohkai

sohkai Apr 9, 2018

Member

Maybe add a comment saying that ganache doesn't like this bit :(.

vault = await getContract('VaultMock').new()
const vaultId = hash('vault.aragonpm.test')
await kernel.setApp(APP_BASE_NAMESPACE, vaultId, vault.address)
await kernel.setDefaultVaultId(vaultId)

This comment has been minimized.

@sohkai

sohkai Apr 9, 2018

Member

These tests should make and use a proxy for the vault rather than the vault's base contract :).

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Apr 10, 2018

I got confused with the namespace for the defaultVaultId during my review.

By default on Kernel initialization we can set vault.aragonid.eth as the default Vault appId. Then when a vault is created and wants to be set as the default Vault it needs to be assigned to the APP_ADDR_NAMESPACE.

We could add a setDefault boolean param to newAppInstance to standardize this:

function newAppInstance(bytes32 _name, address _appBase, bool _setDefault) auth(APP_MANAGER_ROLE, arr(APP_BASES_NAMESPACE, _name)) public returns (ERCProxy appProxy) {
  _setAppIfNew(APP_BASES_NAMESPACE, _name, _appBase);
  appProxy = newAppProxy(this, _name);
  if (_setDefault) setApp(APP_ADDR_NAMESPACE, _name, appProxy);
}

By calling setApp directly and not the internal functions, we make sure the params are checked and it will only succeed if sender has permissions to set something to the namespace.

@izqui
Copy link
Member

izqui left a comment

I want to re-review after Brett's requested changes. I screwed up with my review suggestions 👎😅

Fix default Vault namespace
Address PR #281 comments.
@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 10, 2018

We could add a setDefault boolean param to newAppInstance to standardize this

I like this.

@bingen

This comment has been minimized.

Copy link
Contributor

bingen commented Apr 10, 2018

Yes, this is done in the last commit.

pragma solidity 0.4.18;


contract EtherToken {

This comment has been minimized.

@sohkai

sohkai Apr 13, 2018

Member

Didn't think about this earlier, but maybe this should be renamed to EtherTokenConstant or similar, since FundProxy is EtherToken looks really confusing haha.

Rename EtherToken to EtherTokenConstant
Plus minor linter fix.
@izqui

This comment has been minimized.

Copy link
Member

izqui commented Apr 15, 2018

Why is 'EtherTokenConstantinlib/`?

@bingen

This comment has been minimized.

Copy link
Contributor

bingen commented Apr 15, 2018

Where do you want me to put it? IT's going to be used from aragon-apps too, (and how know where else eventually), so I thought a generic place would be good. But let me know which you think is the best place and I'll move it.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 15, 2018

I'll move 'EtherTokenConstant to common/ since it'll get used by users.

@sohkai sohkai added the audit: whg label Apr 16, 2018

Issue275 vault recoverable (#294)
* DelegateProxy: implement ERCProxy

* Add IsContract

* Refactor FundsProxy into DepositableDelegateProxy and VaultRecoverable

* AppProxyPinned: remove unnecessary fallback

* Coverage: Add IVaultRecoverable to skipped interfaces

* Rename some tests for clarity
* @return AppProxy instance
*/
function newPinnedAppInstance(bytes32 _name, address _appBase) auth(APP_MANAGER_ROLE, arr(APP_BASES_NAMESPACE, _name)) public returns (ERCProxy appProxy) {
function newPinnedAppInstance(bytes32 _name, address _appBase, bool _setDefault) auth(APP_MANAGER_ROLE, arr(APP_BASES_NAMESPACE, _name)) public returns (ERCProxy appProxy) {

This comment has been minimized.

@sohkai

sohkai Apr 19, 2018

Member

@bingen I'm thinking it might be nice to have an overloaded version of newAppInstance() and newPinnedAppInstance() that just passes false to setDefault to avoid making this a breaking change.

krebernisak added a commit to runningbeta/r8-app that referenced this pull request Apr 24, 2018

@sohkai

sohkai approved these changes Apr 25, 2018

@izqui
Copy link
Member

izqui left a comment

Good job!!!! Just some tiny stuff and good to go :)


if (_token == ETH) {
// solium-disable-next-line security/no-call-value
require(vault.call.value(address(this).balance)());

This comment has been minimized.

@izqui

izqui Apr 27, 2018

Member

why do we do a low-level call here? why notrequire(vault.send(this.balance))?

This comment has been minimized.

@bingen

bingen Apr 27, 2018

Contributor

Ok, changing it, but should I rather use transfer?

require(vault.call.value(address(this).balance)());
} else {
uint256 amount = ERC20(_token).balanceOf(this);
ERC20(_token).transfer(vault, amount);

This comment has been minimized.

@izqui

izqui Apr 27, 2018

Member

This is fine in a ERC20 world, I'm working on how the vault can provide multi-standard deposit logic for contracts.

Since this is no longer logic at the proxy level but upgradeable (right?) we can update in the future.

This comment has been minimized.

@bingen

bingen Apr 27, 2018

Contributor

VaultRecoverable is inherited in AragonApp and Kernel, so yes, it should be upgradeable.

This comment has been minimized.

@sohkai

sohkai Apr 28, 2018

Member

Yes, this is in the base logic contracts, not the proxies.



interface DelegateScriptTarget {
function exec() public returns (bool);
}


contract DelegateScript is IEVMScriptExecutor {
contract DelegateScript is IEVMScriptExecutor, IsContract {

This comment has been minimized.

@izqui
// Hardocde constant to save gas
//bytes32 constant public APP_MANAGER_ROLE = keccak256("APP_MANAGER_ROLE");
bytes32 constant public APP_MANAGER_ROLE = 0xb6d92708f3d4817afc106147d969e229ced5c46e65e0a5002a0d391287762bd0;
bytes32 constant public DEFAULT_VAULT_ID = 0x4214e5fd6d0170d69ea641b5614f5093ebecc9928af51e95685c87617489800e;

This comment has been minimized.

@izqui

izqui Apr 27, 2018

Member

Please lets add a comment here

_setAppIfNew(APP_BASES_NAMESPACE, _name, _appBase);
appProxy = newAppProxy(this, _name);
// By calling setApp directly and not the internal functions, we make sure the params are checked
// and it will only succeed if sender has permissions to set something to the namespace.
if (_setDefault) setApp(APP_ADDR_NAMESPACE, _name, appProxy);

This comment has been minimized.

@izqui

izqui Apr 27, 2018

Member

I love how this looks but we need to be consistent with the agreed style:

if (x) {
  y()
}

This comment has been minimized.

@sohkai

sohkai Apr 28, 2018

Member

@bingen Let's force braces as the accepted standard from now. This was inconsistent before so just it keep it in mind when we change code :).

This comment has been minimized.

@bingen

bingen Apr 28, 2018

Contributor

I'm fine with any standard you like, but I think it should be documented somewhere

if (_setDefault) setApp(APP_ADDR_NAMESPACE, _name, appProxy);
}

function newPinnedAppInstance(bytes32 _name, address _appBase) auth(APP_MANAGER_ROLE, arr(APP_BASES_NAMESPACE, _name)) public returns (ERCProxy appProxy) {

This comment has been minimized.

@izqui

izqui Apr 27, 2018

Member

This function needs proper comments :)

@@ -0,0 +1,165 @@
const { assertRevert } = require('./helpers/assertThrow')

This comment has been minimized.

@izqui

izqui Apr 27, 2018

Member

Can we rename the test file to something a bit more descriptive like proxy_recover_funds?

bingen and others added some commits Apr 29, 2018

Merge pull request #300 from aragon/cond-recover
Implement and test conditional recoverability

@bingen bingen merged commit 5f24c49 into dev May 13, 2018

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 99.379%
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the issue275 branch May 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment