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

Asset Pool Underwriter Token (COV) #6

Merged
merged 5 commits into from
Apr 2, 2021
Merged

Asset Pool Underwriter Token (COV) #6

merged 5 commits into from
Apr 2, 2021

Conversation

pdyraga
Copy link
Member

@pdyraga pdyraga commented Mar 24, 2021

Underwriter tokens represent an ownership share in the underlying
collateral of the asset-specific pool. Underwriter tokens are minted
when a user deposits ERC20 tokens into asset-specific pool and they are
burned when a user exits the position. Underwriter tokens natively
support meta transactions. Users can authorize a transfer of their
underwriter tokens with a signature conforming EIP712 standard,
rather than an on-chain transaction from their address just like in the
case of Uniswap LP or DAI token. Anyone can submit this signature
on the user's behalf by calling the permit function, as specified in
EIP2612 standard, paying gas fees, and possibly performing other
actions in the same transaction.

The implementation mostly mirrors UniswapV2ERC20 contract adding
some tweaks from OpenZeppelin ERC20 implementation such as
meaningful revert messages and Approval event emitted on calls to
transferFrom. This allows applications to reconstruct the allowance
for all accounts just by listening to said events. Other
implementations of the EIP may not emit these events, as it isn't
required by the specification and UniswapV2ERC20 is one of them.

Unit tests were based on OpenZeppelin ERC20 unit tests with our own
tests implementation for EIP2612 and EIP712 functions.

Last but not least, this token contract can be cloned using EIP1167
mechanism.

Underwriter tokens represent an ownership share in the underlying
collateral of the asset-specific pool. Underwriter tokens are minted
when a user deposits ERC20 tokens into asset-specific pool and they are
burned when a user exits the position. Underwriter tokens natively
support meta transactions. Users can authorize a transfer of their
underwriter tokens with a signature conforming EIP712 standard,
rather than an on-chain transaction from their address. Anyone can
submit this signature on the user's behalf by calling the permit
function, as specified in EIP2612 standard, paying gas fees, and
possibly performing other actions in the same transaction.

The implementation mostly mirrors UniswapV2ERC20 contract adding
some tweaks from OpenZeppelin ERC20 implementation such as
meaningful revert messages and Approval event emitted on calls to
transferFrom. This allows applications to reconstruct the allowance
for all accounts just by listening to said events. Other
implementations of the EIP may not emit these events, as it isn't
required by the specification and UniswapV2ERC20 is one of them.

Unit tests were based on OpenZeppelin ERC20 unit tests with our own
tests implementation for EIP2612 and EIP712 functions.

Last but not least, this token contract can be cloned using EIP1167
mechanism.
@pdyraga
Copy link
Member Author

pdyraga commented Mar 24, 2021

Open questions (things I do not want to address in this PR):

There is a problem with ethlint and override keyword. When I try to lint UnderwriterToken contract, linter reports syntax error on fields with override. ethlint does not look like an actively maintained project and we might consider switching to solhint. Please compare:
https://github.com/duaraghav8/Ethlint/releases
https://github.com/protofire/solhint/releases

There is an open question on how Asset-specific pools and their corresponding COV tokens will be created. The implementation proposed in this PR allows for EIP1167 cloning but we may want to use CREATE2 opcode. Asset-specific pool creation may be more expensive with CREATE2 but calls to the token and to the pool may be less expensive later as they would not require a DELEGATECALL. Something to decide about later. FWIW, Uniswap uses CREATE2 to create a new pair.

@pdyraga pdyraga requested review from dimpar and mhluongo March 24, 2021 11:48
@pdyraga pdyraga force-pushed the underwriter-token branch from 5cfc129 to d6a0018 Compare March 25, 2021 15:31
@pdyraga pdyraga changed the title Asset-specific pool underwriter token (COV) Asset Pool Underwriter Token (COV) Mar 25, 2021
@pdyraga pdyraga mentioned this pull request Mar 25, 2021
test/UnderwriterTokenTest.js Show resolved Hide resolved
contracts/UnderwriterToken.sol Show resolved Hide resolved
test/UnderwriterTokenTest.js Outdated Show resolved Hide resolved
test/UnderwriterTokenTest.js Outdated Show resolved Hide resolved
pdyraga added 2 commits March 26, 2021 20:26
ethers will be imported from hardhat.config.js via require to
hardhat-waffle.
It is easier to understand now, we shouldn't have it buried too deep in
assertions.
Copy link
Member

@mhluongo mhluongo left a comment

Choose a reason for hiding this comment

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

Big question I have here — why not make use of the OpenZeppelin ERC-20 helpers?

@pdyraga
Copy link
Member Author

pdyraga commented Apr 1, 2021

Big question I have here — why not make use of the OpenZeppelin ERC-20 helpers?

I should make it more clear in the PR description, sorry. I wanted to add support for EIP2612 permit function based on EIP712 signed approvals. Permit function is supported in Dai and in Uniswap LP and allows to, for example, pay for transactions in Dai, see https://github.com/makerdao/developerguides/blob/master/dai/how-to-use-permit-function/how-to-use-permit-function.md.

I do not think permit will be used from day one after we deploy coverage pools but it can improve UX of all dApps that will be integrating with coverage pools in the future and enable new possibilities we are not yet aware of.

OpenZeppelin have quite recently added permit to their ERC20 but this code is in a draft state, was released in the most recent v4.0.0, and to my understanding was not audited yet, and can have breaking changes. On the contrary, Uniswap implementation was audited and is even referenced from the EIP as an implementation example.

I was on the fence and finally decided to copy (audited) Uniswap implementation and do two changes in other (non-permit) functionalities: add revert messages for SafeMath reverts and zero addresses just like they are done in OpenZeppelin, plus add a way to create a token with clone factory (waived in the next PR as a premature optimization). I was not happy with Uniswap tests for the token so decided to port OpenZeppelin tests to satisfy my internal need for paranoid test coverage and implemented more test cases for EIP2612 permit as Uniswap has only one test for this functionality. For what is worth, I was also trying to extend v3 OpenZeppelin ERC20 and add permit functionality copy-pasting it from Uniswap implementation but it does not feel right given that I had to overwrite some OpenZeppelin functions to provide support for uint256(-1) permanent approval mentioned in EIP2612 but not supported by ERC20 approve.

tl;dr; This is an audited, EIP2612-referenced, version with better revert messages and much better test coverage.

pdyraga added 2 commits April 1, 2021 14:02
Technically, it should not matter which action is done first as if there
is not enough allowance the transaction will revert. This is how
OpenZeppelin contract is implemented. However, given that
UnderwriterToken is based on an audited Uniswap LP contract code and
that in the Uniswap contract, allowance is checked before transfer, it
makes sense to keep the order aligned.
@dimpar
Copy link
Contributor

dimpar commented Apr 2, 2021

No more comments other than testing structure might change based on this thread. But also we can refactor it later.

@mhluongo
Copy link
Member

mhluongo commented Apr 2, 2021

👍 on your rationale @pdyraga, permit() is a big UX win. Custom tokens just trigger my audit spidey senses a bit. Something to remember when we bring in reviewers

@mhluongo mhluongo merged commit 1e8dfda into main Apr 2, 2021
@pdyraga pdyraga deleted the underwriter-token branch April 2, 2021 15:08
@pdyraga pdyraga added this to the solidity/v1.0.0 milestone Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants