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

Withdraw helper contract to support collecting funds from channels #454

Merged
merged 5 commits into from
May 4, 2021

Conversation

abarmat
Copy link
Contributor

@abarmat abarmat commented Mar 12, 2021

Summary

Introduces a contract called GRTWithdrawHelper. The main goal of this contract is to encode custom logic in the process of withdrawing funds from the Vector Channel Multisigs. The contract receives funds from a Channel Multisig, then it forwards them to the Staking contract related to an Allocation.

Features

  • The GRTWithdrawHelper is deployed with the token address that will be used for transfers, in our case it is the GRT token address. It can't be changed at later stage but this can easily be solved deploying a new contract if needed.
  • The contract expects to have available tokens in balance when execute(WithdrawData calldata wd, uint256 actualAmount) is called by the Channel. For that to happen the Channel needs to transfer funds to the WithdrawHelper before execute is called.
  • On execute the WithdrawHelper will approve the funds to send to the Staking contract and then execute collect() passing the proper allocationID and amount. The Staking contract will then pull the tokens.
  • If the call to collect() fails, the funds are returned to the channelAddress. This is important for Vector accounting of funds.

Requirements

  • Set the GRTWithdrawHelper contract as AssetHolder in the Staking contract. This way we allow accepting funds from this contract.
  • The cooperative withdrawal commitments must have the following data (indicated with arrows):
struct WithdrawData {
    address channelAddress;
    address assetId;  // -> GRT token address
    address payable recipient; // -> GRTWithdrawHelper contract address
    uint256 amount;
    uint256 nonce;
    address callTo; // -> GRTWithdrawHelper contract address
    bytes callData; // -> CollectData struct defined below
}
struct CollectData {
    address staking;  // -> Staking contract address
    address allocationID; // -> AllocationID to send collected funds
}

The parties must agree and sign the WithdrawalCommitment with that information to achieve proper execution of the withdrawals.

Exceptions

The following conditions will result in reverts of the execution:

  • assetId in WithdrawData not matching the token address configured in the GRTWithdrawHelper.
  • GRTWithdrawHelper not having enough balance to transfer the Staking contract as stated in actualAmount.
  • Setting an invalid staking contract address.

Note: To avoid unexpected conditions in the Vector offchain logic, it is encouraged that the parties verify the WithdrawData does not revert by doing a call/estimateGas.

Handled Exceptions

These reverts are handled and will make the contract to return the funds to the channel address.

  • Doing a withdrawal on a non-existing allocationID as defined in CollectData when calling collect().

Known Behaviour

  • This contract does not know if a WithdrawalCommitment is right or wrong. It just accepts a transfer and WithdrawData that defines a destination.
  • A properly formatted WithdrawData can be crafted to send outside funds to the Staking contract. In that case, the person will get those funds distributed with Curators + Delegators + taxed a protocol percentage. There is no rational economic incentive to do so.

);

// Call the staking contract to collect funds from this contract
IStaking(collectData.staking).collect(_actualAmount, collectData.allocationID);
Copy link

Choose a reason for hiding this comment

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

If this function reverts, the channel funds will be gone. We need to try-catch this call and provide a backup address the funds can be sent to (can even be the channel multisig address which is already part of the withdraw data).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The collect function will only revert if: the allocation ID passed id 0x0 or invalid, it will then accept funds in any case.

My question is, why is that those funds will be lost? won't the collect call reverting make the transaction fail?

I based this on how the https://github.com/connext/vector-withdraw-helpers/blob/main/contracts/UniswapWithdrawHelper/UniswapWithdrawHelper.sol works.

Choose a reason for hiding this comment

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

The transaction will fail, but the funds are already signed out of the channel offchain, so they are effectively marked as "used".

Choose a reason for hiding this comment

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

@rhlsthrm and if the transfer to the channel is made instead, it will be credited to Bob, even though it might've being made by Alice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rhlsthrm what happens if this transaction fails due to an out of gas exception?

Choose a reason for hiding this comment

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

@rhlsthrm and if the transfer to the channel is made instead, it will be credited to Bob, even though it might've being made by Alice?

Yes, if the transfer is made directly to the channel address, it will be credited to Bob. However, you can call depositAlice. We might be able to program the logic to send to channel vs call depositAlice into the helper itself.

Choose a reason for hiding this comment

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

@rhlsthrm what happens if this transaction fails due to an out of gas exception?

If it fails with hitting the gas limit on the transaction, you can resubmit the withdrawal commitment with a higher gas limit. It's only the case of a permanent failure that the funds are unrecoverable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect

@abarmat abarmat added the enhancement New feature or request label Apr 12, 2021
@abarmat
Copy link
Contributor Author

abarmat commented Apr 23, 2021

Rebased

@abarmat abarmat merged commit e2d326f into master May 4, 2021
@abarmat abarmat deleted the ariel/withdraw-helper branch May 4, 2021 02:05
abarmat added a commit that referenced this pull request May 17, 2021
)

Introduces a contract called GRTWithdrawHelper. The main goal of this contract is to encode custom logic in the process of withdrawing funds from the Vector Channel Multisigs. The contract receives funds from a Channel Multisig, then it forwards them to the Staking contract related to an Allocation.

- Add a withdraw helper contract to support collecting funds from channels.
- Try catch revert on the Staking contract and return funds if that happens.
- Add an explicit parties defined return address and wrap approve call to return funds in case of revert.
abarmat added a commit that referenced this pull request May 17, 2021
)

Introduces a contract called GRTWithdrawHelper. The main goal of this contract is to encode custom logic in the process of withdrawing funds from the Vector Channel Multisigs. The contract receives funds from a Channel Multisig, then it forwards them to the Staking contract related to an Allocation.

- Add a withdraw helper contract to support collecting funds from channels.
- Try catch revert on the Staking contract and return funds if that happens.
- Add an explicit parties defined return address and wrap approve call to return funds in case of revert.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request GIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants