Skip to content
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

API to approve ERC20 on a different contract #321

Merged
merged 7 commits into from Jul 10, 2019

Conversation

Projects
None yet
5 participants
@2color
Copy link
Contributor

commented Jun 4, 2019

Changes

  • Allow passing an extra spender param when making an ERC20 token transaction to approve a different contract to the apps.
    Fixes #309

Example:

To test this, set the spender to an address in the Finance app's intentParams.token object:
https://github.com/aragon/aragon-apps/blob/7d61235044509095db09cf354f38422f0778d4bb/apps/finance/app/src/App.js#L58-L60

2color added some commits Jun 4, 2019

@2color 2color requested review from bpierre and izqui Jun 4, 2019

@osarrouy

This comment has been minimized.

Copy link

commented Jun 4, 2019

It looks all good to me [though I can't officially 'review' the PR cause I dont' have the permission to do so].

@2color 2color marked this pull request as ready for review Jun 5, 2019

@izqui

izqui approved these changes Jun 5, 2019

Copy link
Member

left a comment

Looks good to me!

Wondering if this is something we should warn users in the client about, as frontend upgrades (which aren't approved by the organization) could now change the account that can spend tokens from the user's account, potentially leading to loss of funds cc @luisivan @sohkai

@luisivan

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@izqui can you expand on that? Does that mean that apps can now approve withdrawals from different accounts to the one that the user is logged in with?

@izqui

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@luisivan Once a token allowance is granted, the spender has control over transferring the allowed amount to any other address without user intervention (unless the spender is a smart contract and implements some restrictions for when tokens can be pulled from a user's account).

At the moment, the token parameter for an intent only allows apps to request a token allowance from the user's account to the app's smart contract, so only the app's smart contract can get tokens from the user account. Therefore, changing the code that can get tokens from a user's account requires an on-chain upgrade of the contract.

With this change, an app's frontend can set any address as the spender when creating a token allowance, and a frontend upgrade can change this logic silently, setting another address as the spender.

I suggest that at least we show more information about the token approval in the signer panel, so users can see the address that will be able to get tokens from their account. This is the only info that we are showing right now about an intent performing an approval pretransaction:
 2019-06-05 at 11 44 47 AM

@osarrouy

This comment has been minimized.

Copy link

commented Jun 5, 2019

@luisivan Not it means a user can allow a contract which is not directly the app's underlying contract to transferFrom() ERC20 on behalf of the user.

The good news is that Metamask knows when such a approval happens and makes the transaction pretty clear before you sign it :)

@osarrouy

This comment has been minimized.

Copy link

commented Jun 5, 2019

@izqui Agree about making it clearer for the user. Especially since in our case the contract being approved is still an aragon app [just not the current frontend's underlying contract] so we could easily make this readable like:

This transaction will allow MarketMaker to transfer `XX tokens` on your behalf.

2color and others added some commits Jun 5, 2019

Update packages/aragon-wrapper/src/utils/transactions.js
Co-Authored-By: Jorge Izquierdo <izqui97@gmail.com>
@2color

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

I suggest that at least we show more information about the token approval in the signer panel, so users can see the address that will be able to get tokens from their account. This is the only info that we are showing right now about an intent performing an approval pretransaction:

I agree. I'll give that a go.

@2color

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Added a message in aragon/aragon#825

@luisivan

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I suggest that at least we show more information about the token approval in the signer panel, so users can see the address that will be able to get tokens from their account. This is the only info that we are showing right now about an intent performing an approval pretransaction:

Agree as well, thanks for the details

@stale

This comment has been minimized.

Copy link

commented Jul 5, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for contributing to Aragon! 🦅

@stale stale bot added the abandoned label Jul 5, 2019

@2color 2color removed the abandoned label Jul 8, 2019

sohkai added some commits Jul 10, 2019

@sohkai

sohkai approved these changes Jul 10, 2019

Copy link
Member

left a comment

Looks good 🌟!

I just merged with master and added a commit to restructure the API docs a little bit to make the intents a bit more clear.

@sohkai sohkai merged commit 6c35cec into master Jul 10, 2019

11 of 12 checks passed

coverage/coveralls Coverage decreased (-0.1%) to 50.766%
Details
License Compliance All checks passed.
Details
WIP Ready for review
Details
bootstrap bootstrap
Details
build build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
install install
Details
license/cla Contributor License Agreement is signed.
Details
lint lint
Details
size size
Details
test test
Details

@delete-merged-branch delete-merged-branch bot deleted the custom-approve-spender branch Jul 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.