Skip to content

Comments

rft: mint fee shares at the start#1178

Draft
DhairyaSethi wants to merge 2 commits intodevfrom
rft/mint-fee-shares
Draft

rft: mint fee shares at the start#1178
DhairyaSethi wants to merge 2 commits intodevfrom
rft/mint-fee-shares

Conversation

@DhairyaSethi
Copy link
Member

No description provided.


address oldFeeReceiver = asset.feeReceiver;
if (oldFeeReceiver != config.feeReceiver) {
_mintFeeShares(asset, assetId);
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: rm

@@ -209,7 +209,7 @@
function mintFeeShares(uint256 assetId) external restricted returns (uint256) {
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: determine if we want to keep this around, but doesnt seem necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

We mentioned that if there is no action for a while, we might want to manually trigger this, even with this kind of implementation

uint256 accruedFees
);

event AccrueFees(uint256 indexed assetId, address feeReceiver, uint256 feeShares);
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: we need to add natspec here

return asset.drawn(drawnIndex) + asset.premium(drawnIndex);
}

/// @notice Returns the total added assets for the specified asset.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this comment removed?

return asset.totalAddedAssets(asset.getDrawnIndex());
}

/// @notice Returns the total added assets for the specified asset.
Copy link
Contributor

Choose a reason for hiding this comment

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

this could at the end say "at the specified drawnIndex"

aggregatedOwedRay.fromRayUp() -
asset.realizedFees -
asset.getUnrealizedFees(drawnIndex);
return asset.liquidity + asset.swept + aggregatedOwedRay.fromRayUp();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not subtract out unrealizedFees?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, so we are including the unrealized both in the numerator and denominator now? (both assets and shares)

Copy link
Member Author

Choose a reason for hiding this comment

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

no we will remove unrealised fees from this impl and go back to what we had before (only on denominator)

(aggregatedOwedRayAfter.fromRayUp() - aggregatedOwedRayBefore.fromRayUp()).percentMulDown(
liquidityFee
feeAmount.toSharesDown(
asset.liquidity + asset.swept + aggregatedOwedAfter - feeAmount,
Copy link
Contributor

Choose a reason for hiding this comment

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

subtracted out feeAmount because this should be the share rate before the current action (before these fees are accrued), so this looks good


/// @notice Calculates the amount of fee shares derived from the index growth due to interest accrual.
/// @dev The true liquidity growth is always greater than accrued fees, even with 100.00% liquidity fee.
function getFeeShares(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we specify that we are rounding down (which I think is correct btw) in natspec dev comment?

}
}

/// @notice Calculates the drawn index of a specified asset based on the existing drawn rate and index.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing natspec?

Co-authored-by: Cheyenne Atapour <cheyenneatapour@gmail.com>
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