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

Small chores #14

Merged
merged 8 commits into from
Nov 27, 2023
Merged

Small chores #14

merged 8 commits into from
Nov 27, 2023

Conversation

0xmichalis
Copy link
Member

No description provided.

restFee is already a uint256 so Solidity will
revert in case we try to set a negative number
to it.
@0xmichalis 0xmichalis marked this pull request as draft November 27, 2023 09:35
Copy link
Contributor

@PawelTroka PawelTroka left a comment

Choose a reason for hiding this comment

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

lgtm!

@0xmichalis
Copy link
Member Author

@PawelTroka sorry, I've pushed a couple of more changes, ptal

@0xmichalis 0xmichalis marked this pull request as ready for review November 27, 2023 10:37
@@ -30,6 +30,10 @@ jobs:
forge build --sizes --via-ir
id: build

- name: Check formatting
Copy link
Contributor

Choose a reason for hiding this comment

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

good thinking!

/// @return recipients The addresses of the fee recipients.
/// @return feesDenominatedInPoolTokens The amount of fees each recipient should receive.
function calculateRedemptionFee(address tco2, address pool, uint256 depositAmount)
function calculateRedemptionFees(address tco2, address pool, uint256 redemptionAmount)
Copy link
Contributor

Choose a reason for hiding this comment

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

cool, I also wanted to change that :)

} catch Error(string memory reason) {
assertEq("Fee must be greater than 0", reason);
}
vm.expectRevert("Fee must be greater than 0");
Copy link
Contributor

Choose a reason for hiding this comment

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

looks much better!

Copy link
Contributor

@PawelTroka PawelTroka left a comment

Choose a reason for hiding this comment

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

good job here 👍

@0xmichalis 0xmichalis merged commit e920562 into main Nov 27, 2023
1 check passed
@0xmichalis 0xmichalis deleted the chores branch November 27, 2023 12:54
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