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

Updating from PR #13

Closed
wants to merge 6 commits into from
Closed

Updating from PR #13

wants to merge 6 commits into from

Conversation

leeftk
Copy link
Collaborator

@leeftk leeftk commented Jan 13, 2025

PR: Refactor FM_PC_ExternalPrice_Redeeming_v1

Changes:

  • Restructured file organization for better maintainability
  • Fixed fee amount calculation bug
  • Added dedicated internal functions for token redemption and issuance
  • Fixed projectFeeAmount variable handling

Testing:

  • All tests passing
  • No breaking changes to existing functionality

@@ -119,28 +119,34 @@ contract LM_ManualExternalPriceSetter_v1 is
// -------------------------------------------------------------------------
// External Functions

/// @notice Internal function to set the issuance price
/// @param price_ The price to set
function _setIssuancePrice(uint price_) internal {

Choose a reason for hiding this comment

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

these two functions seem to be under // External Functions

@@ -578,7 +578,7 @@ contract FM_PC_ExternalPrice_Redeeming_v1 is

// Create and emit the order.
_createAndEmitOrder(
_receiver, _depositAmount, collateralRedeemAmount, issuanceFeeAmount
_receiver, _depositAmount, collateralRedeemAmount, projectFeeAmount

Choose a reason for hiding this comment

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

what was the reason for this change? just curious

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was from the audit check here

InverterNetwork#704 (comment)

Copy link

@vendrell46 vendrell46 Jan 14, 2025

Choose a reason for hiding this comment

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

I guess we can delete the file if we don't need it anymore

Choose a reason for hiding this comment

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

I guess we can delete the file if we don't need it anymore

@Zitzak Zitzak mentioned this pull request Jan 15, 2025
@Zitzak Zitzak closed this Jan 15, 2025
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