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

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

Open
robsecord opened this issue May 2, 2020 · 25 comments
Open

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

robsecord opened this issue May 2, 2020 · 25 comments

Comments

@robsecord
Copy link
Owner

robsecord commented May 2, 2020 •
edited
Loading

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!
@robsecord robsecord changed the title Unit-Tests Charged Particles - Unit-Tests for Solidity Contracts - 250 DAI Bounty May 2, 2020
@robsecord robsecord changed the title Charged Particles - Unit-Tests for Solidity Contracts - 250 DAI Bounty Charged Particles - Unit-Tests for Solidity Contracts May 2, 2020
@gitcoinbot
Copy link

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
Copy link
Contributor

KiChjang commented May 3, 2020 •
edited
Loading

@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
Copy link
Owner Author

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
Copy link
Contributor

KiChjang commented May 5, 2020

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
Copy link
Owner Author

robsecord commented May 5, 2020 •
edited
Loading

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 KiChjang mentioned this issue May 9, 2020
Merged
@KiChjang
Copy link
Contributor

KiChjang commented May 9, 2020

@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
Copy link
Owner Author

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

@KiChjang
Copy link
Contributor

KiChjang commented May 9, 2020

@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
Copy link
Owner Author

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
Copy link
Contributor

KiChjang commented May 10, 2020 •
edited
Loading

@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
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Contributor

@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.

@KiChjang KiChjang mentioned this issue May 31, 2020
Closed
@robsecord
Copy link
Owner Author

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
Copy link
Owner Author

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

@uivlis
Copy link
Contributor

uivlis commented Jun 13, 2020

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
Copy link
Owner Author

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
Copy link

⚡️ 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
Copy link
Contributor

uivlis commented Jun 22, 2020

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
Copy link

⚡️ 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
Copy link

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
Copy link
Contributor

uivlis commented Jul 3, 2020 •
edited
Loading

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
Copy link

⚡️ 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
Copy link
Owner Author

@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

@robsecord robsecord changed the title Charged Particles - Unit-Tests for Solidity Contracts Bounty: Charged Particles - Unit-Tests for Solidity Contracts Jul 5, 2020
@uivlis
Copy link
Contributor

uivlis commented Jul 5, 2020 •
edited
Loading

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@robsecord @KiChjang @uivlis @gitcoinbot