contract suggestions / discussion

[thelostone-mc]

Questions from @QEDK

• what is a program, in reference to: https://github.com/gitcoinco/grants-round/blob/main/packages/contracts/contracts/program/ProgramImplementation.sol#L12

• what is metaptr used for? in reference to: https://github.com/gitcoinco/grants-round/blob/main/packages/contracts/docs/MetaPtrProtocol.md

• might be worthwhile just using a struct here: https://github.com/gitcoinco/grants-round/blob/main/packages/contracts/contracts/votingStrategy/BulkVotingStrategy.sol#L44

• we shouldn't be using safetransfer at all here, it would DoS the rest of the votes: in reference to https://github.com/gitcoinco/grants-round/blob/main/packages/contracts/contracts/votingStrategy/BulkVotingStrategy.sol#L49

• we should not have a dependency on any RPC / API key like this imo: https://github.com/gitcoinco/grants-round/blob/main/packages/contracts/docs/DEPLOY_STEPS.md

• why are we using factory - impl patterns everywhere? are we sure we gain something with this design decision?

• thoughts on adding slither to the workflow?

[thelostone-mc]

what is a program, in reference to: https://github.com/gitcoinco/grants-round/blob/main/packages/contracts/contracts/program/ProgramImplementation.sol#L12

That should be explained here:
https://github.com/gitcoinco/grants-round/tree/main/packages/contracts#terminology
It doesn't do much at this point -> but the crux of it is programs represents an organization which manage multiple rounds over time.
Eg: Gitcoin could be a program which has you and me as it's operators and gitcoin program can inturn create multiple rounds (few managed by me / others managed by you) etc

---

what is metaptr used for

Any offchain storage is stored as MetaPtr.
At this point we are starting with IPFS but down the lane, we'd like to offer the flexibility to allow people to maybe store data in ceramic / etc. If you look at the script / contracts you'd see references of how the metaPtr struct is being used
Have a look at this here
https://github.com/gitcoinco/grants-round/blob/main/packages/contracts/scripts/round/createRound.ts#L56

---

might be worthwhile just using a struct here: https://github.com/gitcoinco/grants-round/blob/main/packages/contracts/contracts/votingStrategy/BulkVotingStrategy.sol#L44

Oh yes let's sync up on this.
We haven't done anything in terms of actually implementing but the goal was to be able to support multiple types of voting algo -> (which means each can have it's own structure of what constitues as a vote)
What you are proposing -> was what I had initially done. Have a look at the diff in this PR
https://github.com/gitcoinco/grants-round/pull/36/files
This was what I had originally defined as the struct
https://user-images.githubusercontent.com/5358146/179218771-462c0dfe-f9f5-4948-a81e-5cf95fa83577.png

---

we shouldn't be using safetransfer at all here, it would DoS the rest of the votes: in reference to https://github.com/gitcoinco/grants-round/blob/main/packages/contracts/contracts/votingStrategy/BulkVotingStrategy.sol#L49

Oh we should def do that if that's the case. i wasn't aware of this. Could you explain how safeTransfer would result in DoS :D

---

we should not have a dependency on any RPC / API key like this imo: https://github.com/gitcoinco/grants-round/blob/main/packages/contracts/docs/DEPLOY_STEPS.md

Could you elaborate on this / suggest alternatives ?
Those scripts were meant for quick testing. Apart from deploying the contracts once -> the scripts would never be used.

---

why are we using factory - impl patterns everywhere? are we sure we gain something with this design decision?

Factory -> is ideally the root contract which is responsible for creating a new Round / Program. Hence the name Factory
Implementation -> is the flavour of round / program being created.
Initially I wanted to avoid adding the Implementation Suffix -> but on the engineering call Andrea suggested one common practice was to add suffix implementation (which i'm not fan of the name -> it does make it easy to differentiate )

---

thoughts on adding slither to the workflow?

Ohhhh i haven't used yet. YES YES YES
We should create a story and have this setup
I'm assuming you're talking about https://github.com/crytic/slither

[QEDK]

Could you explain how safeTransfer would result in DoS :D

safeTransferFrom is a reverting form of transfer, it's a cleaner way to check for call success but if even one transfer reverts, it will revert the entire tx (hence DoS, think where a malicious actor hooks into _callOptionalReturn and returns false)

Could you elaborate on this / suggest alternatives ?

I think it should be RPC_URL and I only meant that we should silence the warnings where they are not used (for e.g. tests) and only emit them in deployment scripts.

Factory -> is ideally the root contract which is responsible for creating a new Round / Program. Hence the name Factory Implementation -> is the flavour of round / program being created.

I understand the idea, I want to understand if factories are more efficient or if this is something that can be resolved with similar optimization in a single contract.

[thelostone-mc]

safeTransferFrom is a reverting form of transfer, it's a cleaner way to check for call success but if even one transfer reverts, it will revert the entire tx (hence DoS, think where a malicious actor hooks into _callOptionalReturn and returns false)

Ah I see what you're saying :O Good callout -> we should change this.

I think it should be RPC_URL and I only meant that we should silence the warnings where they are not used (for e.g. tests) and only emit them in deployment scripts.

Ah i see what you're saying. Good point. Could i trouble you to write up a story for this and we can get it prioritized ?

I understand the idea, I want to understand if factories are more efficient or if this is something that can be resolved with similar optimization in a single contract

Got a draft in mind ? :D
curious to see if we could get away with 1 contract (That was my original intention) but after a few discussions -> we ended up agreeing on the factory way of going about it

[QEDK]

curious to see if we could get away with 1 contract

I am curious as well, complexity in one contract is cheaper than slightly less complexity in multiple contracts given how EVM works, but it really depends on use-case to use-case ofcourse. I am mostly trying to understand if it is actually the most optimal solution for our use-case.

Could i trouble you to write up a story for this and we can get it prioritized ?

Sure, will do, should add that I am generally not good at these things (but feedback is always appreciated).

[thelostone-mc]

Sure, will do, should add that I am generally not good at these things (but feedback is always appreciated).

Haha no worries ! I'm here to lend a hand when needed

[thelostone-mc]

Stories which need to be create @nfrgosselin

Higher Priority

• Integrate https://github.com/crytic/slither and list of vulnerabilities
• Fix vulnerabilities generated from slither report

Lower Priority

• update safeTransferFrom -> transferFrom (contract suggestions / discussion #169 (reply in thread))
• Remove dependency RPC / API keys ( @QEDK will elaborate)
• spike to research alternative to factory approach

[vacekj]

Closing as done