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

updatable burn redeem contract #103

Merged
merged 6 commits into from
Jan 31, 2025
Merged

updatable burn redeem contract #103

merged 6 commits into from
Jan 31, 2025

Conversation

donpdang
Copy link
Contributor

@donpdang donpdang commented Jan 16, 2025

This PR

Add new ERC721BurnRedeemV2 and ERC1155BurnRedeemV2
Has exact same functionality as ERC721BurnRedeem and ERC1155BurnRedeem except the ability to update mintFees and prevent new burns if neccessary

New methods:

  • setBurnFees: set burn fees, takes in burnFee and multiBurnFee. Only admin of the extension can trigger
  • setActive: whether to allow new burns or not. Only admin of the extension can trigger

New tests for setBurnFees and setActive

Screenshots (if applicable)

Ticket

https://linear.app/manifoldxyz/issue/UPDATE-THIS-PART

Self-Review

  • Review and comment PR file changes
  • Description provides context for the changes
  • Tests are included for the PR (required on server changes)

CR Notes

QA Steps

  • [ ]

Copy link

github-actions bot commented Jan 16, 2025

LCOV of commit 019b0ac during Test! #482

Summary coverage rate:
  lines......: 61.4% (2287 of 3723 lines)
  functions..: 64.6% (330 of 511 functions)
  branches...: 43.8% (453 of 1035 branches)

Files changed coverage rate: n/a

Copy link

LCOV of commit 4718fe4 during Test! #477

Summary coverage rate:
  lines......: 10.7% (70 of 652 lines)
  functions..: 20.5% (17 of 83 functions)
  branches...: 4.2% (10 of 237 branches)

Files changed coverage rate: n/a

error InvalidRedeemAmount();
error InvalidPaymentAmount();

error Inactive();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New error that emit if initializeBurnRedeem is called when active is set to `false

* @param burnFee the burn fee
* @param multiBurnFee the multi burn fee
*/
function setBurnFees(uint256 burnFee, uint256 multiBurnFee) external;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New setBurnFees and setActive methods

/**
* See {IBurnRedeemCoreV2-setBurnFees}.
*/
function setBurnFees(uint256 burnFee, uint256 multiBurnFee) external override adminRequired {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New method to set burn fees

/**
* See {IBurnRedeemCoreV2-setActive}.
*/
function setActive(bool _active) external override adminRequired {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new method to set active status

uint256 instanceId,
BurnRedeemParameters calldata burnRedeemParameters
) internal {
if (!active) revert Inactive();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only allow new burns if active is true

vm.stopPrank();
}

function testSetBurnFees() public {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New tests for setting burn fees

vm.stopPrank();
}

function testSetActive() public {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New tests for setting active

vm.stopPrank();
}

function testSetActive() public {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New tests for setting active

}


function testSetBurnFees() public {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New tests for setting bunFees

burnRedeem.updateBurnRedeem(address(creator), 1, params);

// Only admin can set membership address and update fees
vm.startPrank(owner);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New access tests for setBurnFees and setActive

burnRedeem.updateBurnRedeem(address(creator), 1, params, true);

// Only admin can set membership address and update fees
vm.startPrank(owner);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New access tests for setBurnFees and setActive

@johnnyshankman
Copy link
Contributor

will get eyes on this in the morning

@donpdang donpdang merged commit d541eab into main Jan 31, 2025
7 checks passed
@donpdang donpdang deleted the don/burn-redeem-updatable branch January 31, 2025 19:50
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.

2 participants