Closed apbendi closed 4 years ago
I also had to increase gas limits (not sure if it's real or a bug) but I set them to super high numbers. Need to investigate this.
I had a similar issue and created #16 for it. If you have any details to add that would probably be a good place to put them
Some questions/thoughts:
Ownable.sol
and implementing similar (but reduced) functionality manually?else
blocks where it says // TODO: anything?
Pool.sol
: Do we need this file? It's not used. I haven't looked at it closely but I don't remember what it's used fordai.json
in a folder called abi
at the project root, and adding a file called addresses.json
at the project root with all relevant contract address. We'll need those in both the contracts and frontend so probably should have it all in one spot
Otherwise, all tests passed! 🥳
Other general question—in the tests I noticed you do things like this.instance
in the before()
hook, whereas with Truffle tests I (we?) previously used let instance;
just above the before hook and instance = ...
within the hook when creating the contract. Is there any tradeoffs / reasons / considerations here as to the differences in the approaches?
Great feedback. Thanks!
We should probably rename Moloch to Endaoment everywhere
Good call. Done.
What was the rationale for removing Ownable.sol and implementing similar (but reduced) functionality manually?
Good question. So the new OZ contracts all use the "initializer" pattern, which broke Moloch's assumptions around the owner
being set automatically in the constructor. When I realized this, I tried calling the initialize
function but then I got out of gas errors. Didn't seem worth fighting with especially because the Endaoment contract owns the Guildbank and literally can't utilize the rest of OZ's Ownable implementation (unless we expanded it).
If a grant or revocation proposal does not pass, I think we do not need to do anything in the else blocks where it says // TODO: anything?
Don't think so either. Removed.
Pool.sol: Do we need this file? It's not used. I haven't looked at it closely but I don't remember what it's used for
Agreed don't think we need it.
I'd recommend putting dai.json in a folder called abi at the project root, and adding a file called addresses.json at the project root with all relevant contract address. We'll need those in both the contracts and frontend so probably should have it all in one spot
Ahh yes good call, will do this.
Other general question—in the tests I noticed you do things like this.instance in the before() hook, whereas with Truffle tests I (we?) previously used let instance; just above the before hook and instance = ... within the hook when creating the contract. Is there any tradeoffs / reasons / considerations here as to the differences in the approaches?
No major tradeoffs I'm aware of. In fact if my understand of JavaScript is correct they're functionally the same. Not sure why I used this
this time 🤷‍♂️
OK! I've made changes for all your suggestions and rebased to your (now merged) branch. If it looks good to you, feel free to merge!
Basic functionality for issuing & revoking streaming grants via Sablier.
Aside from needing some cleanup & cleaning up the commit history I think this is good to go. I also had to increase gas limits (not sure if it's real or a bug) but I set them to super high numbers. Need to investigate this.
closes #6 closes #7 closes #11