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

RedeemPool, Redeemer proxy contract and tests #790

Merged
merged 9 commits into from
Sep 10, 2019

Conversation

0xsarvesh
Copy link
Contributor

@0xsarvesh 0xsarvesh commented Sep 3, 2019

fixes #788

This PR implements Redeem Composer, Redeemer proxy contract and integration of redeem flow with redeem composer.
These contracts are similar to OST composer and Staker proxy, however there are few deviations.

  1. stakeRequest mapping in the OST composer contract seems redundant. I haven't added this storage in Redeem composer contract

https://github.com/mosaicdao/mosaic-contracts/blob/45023774d47ab8322bfb8cb495175fdf62b8a509/contracts/gateway/OSTComposer.sol#L83

  1. Bounty in co-gateway contract is in base token instead of EIP20 token. AceeptRedeemRequest is a payable function.

@0xsarvesh 0xsarvesh changed the title Draft implementation of RedeemComposer contract Draft implementation of RedeemComposer and Redeemer proxy contract Sep 4, 2019
@0xsarvesh 0xsarvesh marked this pull request as ready for review September 5, 2019 07:50
Copy link
Contributor

@deepesh-kn deepesh-kn left a comment

Choose a reason for hiding this comment

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

Very nice code 👍 .
I scanned through the contract code only, will go through the tests as well, providing the feedback so that you can be unblocked.
Maybe you want to consider implementing the proxy pattern for RedeemProxy to reduce the gas consumption. #773

contracts/gateway/RedeemComposer.sol Outdated Show resolved Hide resolved
@0xsarvesh 0xsarvesh changed the title Draft implementation of RedeemComposer and Redeemer proxy contract RedeemCompose, Redeemer proxy contract and tests Sep 5, 2019
Copy link
Contributor

@deepesh-kn deepesh-kn left a comment

Choose a reason for hiding this comment

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

👍
Very minor cosmetic changes, please see inline.

contracts/gateway/RedeemPool.sol Outdated Show resolved Hide resolved
contracts/gateway/RedeemPool.sol Show resolved Hide resolved
contracts/gateway/RedeemPool.sol Show resolved Hide resolved
contracts/gateway/RedeemPool.sol Outdated Show resolved Hide resolved
contracts/gateway/RedeemPool.sol Show resolved Hide resolved
contracts/gateway/RedeemerProxy.sol Outdated Show resolved Hide resolved
contracts/gateway/RedeemerProxy.sol Show resolved Hide resolved
@0xsarvesh 0xsarvesh changed the title RedeemCompose, Redeemer proxy contract and tests RedeemPool, Redeemer proxy contract and tests Sep 10, 2019
Copy link
Contributor

@deepesh-kn deepesh-kn left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@deepesh-kn deepesh-kn merged commit 20cab4f into OpenST:develop Sep 10, 2019
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.

Redeem composer contract
2 participants