search

robsecord / ChargedParticlesEth

Token Contracts for Ethereum Charged Particles (Interest bearing NFTs)
31 stars 11 forks source link

Bounty: Charged Particles - Unit-Tests for Solidity Contracts #2

Open robsecord opened 4 years ago

robsecord commented 4 years ago

The Solidity Contracts for Charged Particles need Unit-Tests! All Contracts under the ./contracts folder (except DAI.sol)

This bounty is paying 40 DAI for every 10% increase in coverage! Plus a 100 DAI bonus if the coverage reaches 100%!

That's a possible total of 500 DAI!

Requirements:

  1. Fork the repo and create a new branch with your username,
  2. Make note of the current coverage percentage before you start,
  3. commit changes to your branch,
  4. Make note of the coverage percentage after you have completed,
  5. Submit a Pull-Request of your branch for this issue with your submission, noting the starting and ending coverage percentage.

We're up to ~20% coverage! 420 DAI remaining to be claimed!

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 120.0 DAI (120.0 USD @ $1.0/DAI) attached to it as part of the Charged Particles fund.

KiChjang commented 4 years ago

@robsecord I just realized that the current ChargedParticlesERC1155 contract on master does not implement IChargedParticlesERC1155, is that intended?

EDIT: Same question for ChargedParticlesEscrow and IChargedParticlesEscrow.

robsecord commented 4 years ago

Ya, I used the interfaces for something else. Forget what now. I guess I could inherit from them, but it's not necessary. Good catch though, thanks!

KiChjang commented 4 years ago

Update: In the ChargedParticles contract, the majority of the Admin/DAO functions and all of the public read functions for an ion token have unit tests with then, right now I'm trying to create unit tests for minting ion tokens. Later on I will add more for minting a new particle or plasma.

robsecord commented 4 years ago

Thanks for the update! Let me know if you have any questions or need anything from me!

I realize that there is a lot of work involved, so I have updated the original issue post.

Feel free to commit in stages, and I will payout according to the coverage achieved, but please let me know if you do not plan to continue so that I or others can pick up where you left off.

KiChjang commented 4 years ago

@robsecord I noticed one thing -- the withdrawFees function does not set the collectedFees for that particular _contractOwner back to 0. Is this intended, or is it a bug?

robsecord commented 4 years ago

Definitely a bug! Thx for catching it and pointing it out! :)

KiChjang commented 4 years ago

@robsecord I'm still going to write some more unit tests, the last one there was just partially done so that you may be unblocked on other tasks that you may want to use them on.

robsecord commented 4 years ago

Thanks for the update! I have made some slight changes to the file structure. Please pull latest. I have also tried to get coverage working to no avail.. It seems it just doesn't want to work yet.. I have also committed a fix for the withdraw functions that you mentioned. Thanks again for finding that!

KiChjang commented 4 years ago

@robsecord Sorry, the withdrawFees function in ChargedParticles.sol is still not resetting the associated collectedFees back to 0. In the unit tests, it is still impossible to repeatedly withdraw fees indefinitely, because it is checked by Address.sol when you call sendValue, but this doesn't sound like a proper solution to the problem.

robsecord commented 4 years ago

Ahh, I fixed the withdraw functions in the escrow! Didn't realize you meant the one in the controller.. I have now fixed that too, pull latest! Thx again!

robsecord commented 4 years ago

Any update on this? If not it's ok, I will pay out a portion of the amount for the work done so far.

KiChjang commented 4 years ago

@robsecord Hey, sorry I dropped the ball on this one. I had to deal with some personal issues and this slipped my mind. I have a small enhancement on the unit test that I have locally, will make a PR shortly. After that, you may proceed with the pay out.

robsecord commented 4 years ago

Hey, no problem, thx for the work so far, and let me know if you want to pick the ball back up! I want to keep the bounty open in order to get it completed, so I will send you a tip on Gitcoin for the work done. I first need to review what you have just submitted and see if I can get a coverage report working so I can get the numbers. I will keep you posted, should be able to check this out tomorrow evening (eastern timezone).

robsecord commented 4 years ago

Hi uivlis, thx for starting on this! Any update on progress?

uivlis commented 4 years ago

Hey @robsecord, thanks for your patience! Oh, and also for your tip!

Somehow, your comment escaped my notifications, and I saw it only now.

Yes, I started the task, however, I found little to no time to complete it, I've been working mostly to complete my other started bounty, but now that that's done, I can focus on this.

I saw that the project has a remarkable combination of truffle, buidler, jest, openzeppelin, all mashed up together in one big cauldron. Now, from my knowledge, jest can't measure coverage with solidity contracts (neither with truffle, neither with buidler) so, there is no way to measure coverage in the current cauldron configuration (but please correct me if I'm wrong).

Anyway, I'm on it starting now, will keep you updated.

robsecord commented 4 years ago

You're right, I have had a nightmare trying to make Jest + Coverage work.. We can drop Jest and stick with Buidler + Mocha. And I plan to move away from Truffle and go entirely with Buidler (might even switch to ethers.js too), so feel free to tackle this any way you want!

gitcoinbot commented 4 years ago

⚡️ A tip worth 40.00000 DAI (40.0 USD @ $1.0/DAI) has been granted to @kichjang for this issue from @robsecord. ⚡️

Nice work @kichjang! Your tip has automatically been deposited in the ETH address we have on file.

uivlis commented 4 years ago

Hey @robsecord, sorry for this, but I'll have to stop work. This just evades my time and it's better if I let it open to other people. Hopefully, I'll restart work sometime in the future, but thanks again for your patience.

gitcoinbot commented 4 years ago

⚡️ A tip worth 5.00000 DAI (5.0 USD @ $1.0/DAI) has been granted to @uivlis for this issue from @robsecord. ⚡️

Nice work @uivlis! Your tip has automatically been deposited in the ETH address we have on file.

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 266 years, 4 months from now. Please review their action plans below:

1) uivlis has started work.

I just cleaned up some tests, and I may be wrong, but it seems you might have some left-over code which creates errors when minting particles, for example.

Learn more on the Gitcoin Issue Details page.

uivlis commented 4 years ago

Hey @robsecord just went again through the code. You had a typo somewhere in a require (I think), and a lot of functions in escrowMgr that I don't know where and how to call in order for the minting particles test to work. Could you explain a bit that part of contract logic to me, so that I can move the testing forward? You can look at the PR to see my comments added to the functions I was talking about. Also, I upgraded the dependencies, so I had to remove some incompatible logging configuration somewhere in the deployment scripts.

gitcoinbot commented 4 years ago

⚡️ A tip worth 40.00000 DAI (40.0 USD @ $1.0/DAI) has been granted to @uivlis for this issue from @robsecord. ⚡️

Nice work @uivlis! Your tip has automatically been deposited in the ETH address we have on file.

robsecord commented 4 years ago

@uivlis are you on Discord? would be great to chat there, and I can explain things a bit better for you. Otherwise, everything looks good, I left a couple of comments but still merged the PR. Works well, and we're up to 19% coverage so I tipped you the 40 DAI for increasing coverage to ~20%.

Charged Particles on Discord: https://discord.gg/Syh3gjz

uivlis commented 4 years ago

Thanks for the tip, I just joined the server. You can DM me, or else I'll have to wait some minutes until I can post in the bounties channel.