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

feat: Initial approach to support nexus integration #235

Merged
merged 14 commits into from
Jan 25, 2022

Conversation

satyamakgec
Copy link
Contributor

@satyamakgec satyamakgec commented Dec 14, 2021

Note - I used very layman terminology in the code to make it more understandable although the code is very straightforward. Open for the naming convention change 😜

  • Test case (Will start once I got sufficient approval on the approach).
  • Linting fixes

@coveralls
Copy link

coveralls commented Dec 14, 2021

Coverage Status

Coverage decreased (-0.4%) to 93.75% when pulling cd761ab on feature-nexus-mutual-support into 65fddc8 on main.

Copy link
Contributor

@jrhea jrhea left a comment

Choose a reason for hiding this comment

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

Ok made an initial pass on the code. It looks good just a few suggestions/questions/comments

contracts/external/nexus/WrappedFuturesPrincipal.sol Outdated Show resolved Hide resolved
contracts/external/nexus/WrappedFuturesPrincipal.sol Outdated Show resolved Hide resolved
contracts/external/nexus/WrappedFuturesPrincipal.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@NicholasDotSol NicholasDotSol left a comment

Choose a reason for hiding this comment

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

The approach looks good, it seems to match what the nexus folks are expecting

contracts/external/nexus/WrappedCoveredPrincipalToken.sol Outdated Show resolved Hide resolved
contracts/external/nexus/WrappedCoveredPrincipalToken.sol Outdated Show resolved Hide resolved
contracts/external/nexus/WrappedCoveredPrincipalToken.sol Outdated Show resolved Hide resolved
@satyamakgec satyamakgec marked this pull request as ready for review December 22, 2021 08:23
IERC20(_tranche).safeTransferFrom(
msg.sender,
address(this),
_fromWad(_amount, _tranche)
Copy link
Contributor

Choose a reason for hiding this comment

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

This conversion unnecessary because the constructor of the tranche locks the decimals of the tranche to be equal to that of underlying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is needed as there is a chance where tranche and wrapped covered principal token have different decimals.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that possible? Perhaps we should set to match the underlying as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I think we can only match it with the baseToken decimals. Although I am agnostic for this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should match it with the base decimal tokens 100% of the time.

Copy link
Contributor

@aleph-v aleph-v left a comment

Choose a reason for hiding this comment

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

Looks good to go

contracts/external/nexus/WrappedCoveredPrincipalToken.sol Outdated Show resolved Hide resolved
/// @notice Reclaim tranche token (i.e principal token) by the authorized account.
/// @param _expiration Timestamp at which the derived tranche would get expired.
/// @param _wrappedPosition Address of the Wrapped position which is used to derive the tranche.
function reclaimPt(uint256 _expiration, address _wrappedPosition)
Copy link

Choose a reason for hiding this comment

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

I think this function should burn sender's balance and send them the corresponding amount of underlying. As it stands, the RECLAIM_ROLE can just rug the whole underlying at any moment.

I expect RECLAIM_ROLE to be given to a third party like Nexus mutual so I don't want this trust in the system.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this makes sense. The wrapping is a one way function so you can't really 'rug' the backing token because a wrappers can't get any funds out anyway. Also we don't want to require Nexus to have the tokens because we want to give them max flexibility to redeem even if their architecture means a sc without generic call holds the tokens.

Copy link

@maxsam4 maxsam4 Jan 14, 2022

Choose a reason for hiding this comment

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

The wrapping is a one way function so you can't really 'rug' the backing token because a wrappers can't get any funds out anyway.

This function sends the tranche tokens to the user. aka it is unwrapping the wrapped tokens without consuming the wrapped tokens.

Also we don't want to require Nexus to have the tokens because we want to give them max flexibility to redeem even if their architecture means a sc without generic call holds the tokens.

If we are ok with this trust in the system, this comment can be ignored.

@satyamakgec satyamakgec merged commit 8856664 into main Jan 25, 2022
@satyamakgec satyamakgec deleted the feature-nexus-mutual-support branch January 25, 2022 15:49
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.

6 participants