Smart Escrow contract review notes

--- tags: Smart Invoice, escrow --- # Smart Escrow contract review notes https://github.com/raid-guild/smart-escrow/tree/main/packages/contracts/contracts ## First Review - 4/27/21 ### WrappedInvoiceFactory.sol - what's the purpose of the `LogNewInvoice` event; don't we already have that from `SmartInvoiceFactory.sol`? Or do we need to track new wrapped invoices, too? If so, perhaps rename to `LogNewWrappedInvoice` or something > Yes it will definitely be useful to track new wrapped invoices. Renamed as suggested. ### WrappedInvoice.sol - in `init`, would it be more gas efficient to do all the require statements first before any storage setting? For example, in the case that that the child/parent/invoice addresses are all valid but the split is not, 5 storage variables worth of gas will have been irrevocably spent. > Of course! - I'm not sure I understand how `onlyRaider` works. The way its currently defined it looks like it requires that msg.sender be the DAO or the multisig itself. Is that the intended logic? Or is the idea that any individual raider would qualify? > Only the DAO or the Multisig will have access to disperse funds - So the `disperse` and `disperseAll` functions would distribute raider funds to individual raiders without first sending the funds to the multisig? If so, then I think I better understand why `onlyRaider` is defined the way it is. Overall, I don't think this is a top priority, but it could be a nice feature that could connect to further expansions of this concept (as discussed earlier today). > Yes that is the idea. We can reduce the number of transactions by distributing the funds directly from this contract. - Also, why are they "override"? > Because I've written interfaces and every method that overrides an interface method must have the "override" > ok cool [name=spengrah] - If `lock` is `onlyRaider`, then how would the client call `lock` on the invoice? Would the front end directly expose the `SmartInvoice.sol` interface for clients? > Yes thats the idea. We don't need extra functionality on the WrappedInvoice than necessary I thought. It doesn't need to know who the client is. In any case, for all other purposes such as Deposit, Release, etc we need to directly expose SmartInvoice. > I guess the trade off is more consise contracts vs. easier front end implementation. How much easier on the front end dev would it be to only have to expose a single contract? > Maybe another way to ask this question is why would we need `WrappedInvoice.lock` when we don't *need* `WrappedInvoice.depost/release/etc`? > What I'm wondering is if we should added all external functions from `SmartInvoice.sol` into `WrappedInvoice.sol`. We probably wouldn't even need require statements on any of them since those checks are taken care of by `SmartInvoice.sol` > [name=spengrah] ### General - can you comment who the parent and child are? I know parent = RG and child = raiders, but it would be helpful to clarify throughout the code > Done - btw I found this which sounds pretty cool and useful: https://github.com/tryethernal/hardhat-ethernal > Integrated ## Second Review - 5/2/21 - I like the simplification from `_splitRatio` to `_splitFactor`. Slightly less flexible/generalizable but we really don't need to support much beyond our basic spoils use case, so its definitely worth getting rid of the extra code complexity and the required library. - `_disperse` is pretty awesome! Lotta cool raid distro UI stuff can be built on top of that :slightly_smiling_face: - I checked out the tests briefly and they look good too. Though I did need to modify the `hardhat.config` to get tests to run. I think the issue was the `.env` that I wasn't using, plus maybe that the `hardhat` network wasn't listed.