search

mowblox / emt-marketplace

The goal is to help people become really good at what they do through decentralized mentorship.
https://emt-marketplace.vercel.app
Apache License 2.0
1 stars 3 forks source link

Written tests for Expert tkn & Mktplace #13

Closed Jovells closed 1 year ago

Jovells commented 1 year ago

Added "chain" command to Package.json. This command creates a local fork of topos network

Removed forking configuration from hardhat config. Interferes running testing when network is bad.

Wrote tests for Expert Token And MarketPlace contracts

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
emt-marketplace ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 2, 2023 1:26pm
mickeymond commented 1 year ago

@od41 Please do not merge this PR yet.

mickeymond commented 1 year ago

I had to make some of the data definitions in the marketPlace contract public in order to access them for testing

@Jovells I do not agree with making data definitions PUBLIC all in the name of testing them. Because PRIVATE data definitions and functions are not supposed to tested from the outside but they will be covered during the test as long are they are utilized by other PUBLIC definitions.

Jovells commented 1 year ago

I had to make some of the data definitions in the marketPlace contract public in order to access them for testing

@Jovells I do not agree with making data definitions PUBLIC all in the name of testing them. Because PRIVATE data definitions and functions are not supposed to tested from the outside but they will be covered during the test as long are they are utilized by other PUBLIC definitions.

Okay. The variables that I made public were _MENT_TOKEN_ADDRESS, _UPVOTE_WEIGHT and _DOWNVOTE_WEIGHT. I thought making them public would have no impact on the security of the contract. Can you help me appreciate why they need to be kept private?

od41 commented 1 year ago

Hello guys, what's the bottom line on this PR?

@Jovells @mickeymond

mickeymond commented 1 year ago

@Jovells We are making 2 separate arguments here. Whether or not those 3 data definitions should be marked as PUBLIC and their security implications is a valid argument that can be justified by transparency and whatnot. My argument was why make a data definition PUBLIC with the justification of Testing it but now it looks like there is more to marking it as PUBLIC than just testing it like you mentioned earlier on Asana. I haven't thought about the security impact yet but my approach is to leave it as PRIVATE always unless you have a very good reason to make it PUBLIC. @od41 I think you can proceed to merge this PR since we will be ironing some of these out as we go.