Feedback & Suggestions

[MattPereira]

Overview

This issue tracks feedback and ideas spawned by my attempt to take down #4

General Thoughts

• Maybe @escottalexander should create branches ahead of time on this repo so we can make PR's at the branches and do reviews for each other. This would also solve problem of permission denied when new challenge creator dev tries to run git push upstream as instructed by the provided challenge issues
• Do we need to keep contract name and file name of Challenge ? From DX perspective, would be nice to name the contracts what they are instead of generic challenge.
• Will the devs be given the arguments to the functions or just the function names?
• Is there room for higher degrees of difficulty within the same challenge? Like you pass if you satisfy some of the tests, but get a higher score if you pass all
• Should we consider challenge where dev writes the tests and we compute score based on coverage %?
• Should we import console into the Challenge contract by default to encourage dev to use it for debugging?
• Should we explain you can run a single test instead of all tests with the --match-test flag? ( matters more for when contracts get complex with many tests )

Token Wrapper Challenge

• The challenge description refers to WrappedETH.sol but the contract name is still Challenge.sol which is confusing
• The short names for function params like wad and dst are difficult to understand
• There are no vm.expectEmits in the tests despite emitting events being a requirement in many of Challenge.sols function natspec

Token Streaming Challenge

At any point during the stream, if they trigger a withdraw, it will send whatever portion is unlocked based on how much time has elapsed

• Does that mean you want all withdraws to be max amount to make it simpler?
• Do we want the same contract to handle both ETH and token streams or should we use 2 seperate contracts?
• Maybe throwing in the ability to transfer ownership of the stream contract would be a nice wrinkle?

[escottalexander]

@MattPereira These are great suggestions and I will implement several of them right away. Really appreciate your insight.

Some of your feedback I have questions about.

General Thoughts

Maybe @escottalexander should create branches ahead of time on this repo so we can make PR's at the branches and do reviews for each other. This would also solve problem of permission denied when new challenge creator dev tries to run git push upstream as instructed by the provided challenge issues

Great idea. I will implement this for sure. https://github.com/BuidlGuidl/eth-tech-tree-challenges/issues/11

Do we need to keep contract name and file name of Challenge ? From DX perspective, would be nice to name the contracts what they are instead of generic challenge.

Great point here as well. We should adjust contract names to match the challenges name. https://github.com/BuidlGuidl/eth-tech-tree-challenges/issues/12

Will the devs be given the arguments to the functions or just the function names?

They will be given the arguments as well unless you can think of a way for us to test their code without knowing the functions and their arguments. I would really love it if we could make it less explicit but I don't see a way to accomplish that while keeping the tests automated. Open to ideas if you have any.

Is there room for higher degrees of difficulty within the same challenge? Like you pass if you satisfy some of the tests, but get a higher score if you pass all

I like this idea. We can mull it over some more. That would be really cool if instead of just PASS/FAIL you got a score based on edge case handling. We can record ideas for general test cases that we should keep in mind here: https://github.com/BuidlGuidl/eth-tech-tree-challenges/issues/13

Should we consider challenge where dev writes the tests and we compute score based on coverage %?

That is a great idea. One existing reference would have them dive in to contract fuzzing so I think this goes nicely in that vein.

Should we import console into the Challenge contract by default to encourage dev to use it for debugging?

Great idea. They will probably need it while debugging so it would be good to have it ready to go. https://github.com/BuidlGuidl/eth-tech-tree-challenges/issues/14

Should we explain you can run a single test instead of all tests with the --match-test flag? ( matters more for when contracts get complex with many tests )

Yes, that is super helpful when people are starting off. Good thinking. https://github.com/BuidlGuidl/eth-tech-tree-challenges/issues/15

[escottalexander]

Token Wrapper Challenge

I am making an issue for addressing everything you mentioned about the token wrapper challenge. https://github.com/BuidlGuidl/eth-tech-tree-challenges/issues/16

[escottalexander]

Token Streaming Challenge

@MattPereira Answering these questions here:

Does that mean you want all withdraws to be max amount to make it simpler?

Yes

Do we want the same contract to handle both ETH and token streams or should we use 2 seperate contracts?

Good question. This boils down to what you think will be easiest for the challenger. This is meant to be a level 1 challenge so lets try not to over-complicate it. I will leave that in your court. If you think that this should be a level 2 challenge then feel free to make that call as well.

Maybe throwing in the ability to transfer ownership of the stream contract would be a nice wrinkle?

I was envisioning that anyone could use the contract to create a stream for anyone. Open to your thoughts on this approach. My design might be a little too complex though for a level 1 challenge. Your functionality suggestion would be a nice touch if the contract only facilitates one stream.

[escottalexander]

Token Wrapper Challenge

I am making an issue for addressing everything you mentioned about the token wrapper challenge. #16

@MattPereira Do you mind checking over this PR? https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/17 Let me know if I need to do anything permissions-wise to allow you to review it.