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.

Sign up for GitHub

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

Jump to bottom

Added test for RewardsReceiver and SharedDepositMinterV2 #18

Merged
merged 16 commits into from
Jun 12, 2024
Merged

Added test for RewardsReceiver and SharedDepositMinterV2 #18

merged 16 commits into from
Jun 12, 2024

Conversation

devlancer412
Copy link
Contributor

No description provided.

@chimera-defi
Copy link
Owner

looks like CI is failing here

  7) SharedDepositMinterV2
       slash:
     Error: Transaction reverted: function selector was not recognized and there's no fallback function

@devlancer412
Copy link
Contributor Author

devlancer412 commented Jun 6, 2024 •
edited
Loading

image
Actually I also checked this action. But all test cases are working well on my side. Could you check it on your local?

And seems like it is being reverted at this point
image

But I deployed FeeCalc contract correctly and there aren't any reason to be reverted
image

@chimera-defi
Copy link
Owner

hmm are you sure you uploaded all files? maybe missing local changes?

chimera-defi
chimera-defi reviewed Jun 6, 2024
View reviewed changes
test/v2/core/minter.spec.js Outdated
const numValidators = 1000;
const adminFee = 0;

const FeeCalc = await ethers.getContractFactory("FeeCalc");
Copy link
Owner

Choose a reason for hiding this comment

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

try to use the zero address for this like the deploy instead.
the idea was to flesh out and launch a fee calc contract for configuring fees later. but it should default to running internal logic if the passed in address is address(0)

@devlancer412 devlancer412 requested a review from chimera-defi June 6, 2024 05:00
chimera-defi
chimera-defi requested changes Jun 7, 2024
View reviewed changes
test/v2/core/minter.spec.js Outdated
expect(afterStake).to.eq(prevStake.add(parseEther("1")));
});

it("depositAndStakeFor", async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

can we add a test to make sure this works for the direction case?
i.e. Alice calls the function, but with Bobs address, and bob recvs the tokens in the end

test/v2/core/minter.spec.js Outdated
expect(afterBalance).to.eq(prevBalance.sub(parseEther("0.5")));

prevBalance = await sgEth.balanceOf(alice.address);
await minter.connect(alice).withdrawTo(parseEther("0.5"), alice.address);
Copy link
Owner

Choose a reason for hiding this comment

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

same as above, can we test this works for directing rewards to others?

test/v2/core/minter.spec.js

expect(afterBalance).to.eq(prevBalance.sub(parseEther("0.5")));
});

Copy link
Owner

Choose a reason for hiding this comment

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

can we add a comment here denoting all the functions below are Admin or Keeper functions?
And that we are checking that the auth is safe on them?

@chimera-defi
Copy link
Owner

are we missing anything obvious here in the tests? weird its not showing more like 100% cov

File                                          |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |

 contracts/v2/core/                           |    66.67 |    46.81 |    68.89 |    61.54 |                |
  RewardsReceiver.sol                         |    83.33 |       50 |       60 |    77.78 |          42,48 |
  Rollover.sol                                |        0 |        0 |        0 |        0 |    28,33,34,37 |
  SgETH.sol                                   |      100 |      100 |      100 |      100 |                |
  SharedDepositMinterV2.sol                   |    73.81 |       50 |    79.17 |    65.71 |... 260,262,266 |

@devlancer412
Copy link
Contributor Author

Actually I'm not sure what should I test on Rollover contract. it's just have one internal function.
About Minter contract, I couldn't test some functions because it's connected to StakingContract(seems like it is external contract).
About RewardsReceiver, I tested all. seems like another functions are ownable functions such as transferOwner, renounceOwnaship

@chimera-defi
Copy link
Owner

ah ok, can you add tests for the owner functions, check to see they work only for the owner? @devlancer412
other than that, just comments on the code review

chimera-defi
chimera-defi reviewed Jun 8, 2024
View reviewed changes
test/v2/core/minter.spec.js Outdated
});

it("slash", async () => {
const GOV_ROLE = await minter.GOV();
Copy link
Owner

Choose a reason for hiding this comment

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

example:

  • user deposits
  • rewards generated
  • reward sync called
  • slash called
  • user redeems
    ensure user does not get back >1

@devlancer412
Copy link
Contributor Author

@chimera-defi Please check last updates

@chimera-defi
Copy link
Owner

nice, can we fix the build plz? @devlancer412

@devlancer412
Copy link
Contributor Author

@chimera-defi merged another test pr to here

chimera-defi
chimera-defi approved these changes Jun 11, 2024
View reviewed changes
contracts/lib/xERC4626.sol

storedTotalAssets = storedTotalAssets_ + lastRewardAmount_; // SSTORE
if (end - timestamp < rewardsCycleLength / 20) {
Copy link
Owner

Choose a reason for hiding this comment

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

whats with the changes here? can we reset this file to head?
ill add any auto lint stuff later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't have any changes on this file. all changes are from prettier such as removing spaces

@chimera-defi chimera-defi changed the base branch from main to ch/contractImprovements June 12, 2024 00:46
@chimera-defi chimera-defi merged commit a999e52 into chimera-defi:ch/contractImprovements Jun 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chimera-defi chimera-defi chimera-defi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@devlancer412 @chimera-defi