General contract review

[solomondefi-dev]

Look at the entire contract system and try to simplify and clean up where possible.

[jtse0]

After a total review of the contracts and updating of all the tests just a few questions popped up:

• Are the discounts for the chargebacks and preorders intended solely for record purposes only?
• SlmEscrow - functions for submission of party 1 and party 2 evidence were removed in favor of the one initiateDispute() function that can serve both parties. Is this alright?

[solomondefi-dev]

Are the discounts for the chargebacks and preorders intended solely for record purposes only?

The intent of this is to provide a discount for buyers that use SLM (instead of ETH, or another supported token). Can you create a new issue for this? You can see the initialization in SlmFactory

SlmEscrow - functions for submission of party 1 and party 2 evidence were removed in favor of the one initiateDispute() function that can serve both parties. Is this alright?

Can you clarify? I still see the party1Evidence and party2Evidence functions in SlmEscrow. The difference is that initiateDispute in SlmEscrow allows the initializer to provide an evidence URL.

[jtse0]

Alright I can make a new issue for the discounts. I saw that the discounts are part of the initialization, however I was just wondering about the actual use since we are just initializing the value and not using it elsewhere. I guess in the new issue we can discuss more about how the discounts will impact calculations.

As for the second point - there originally were three functions available for parties to upload their evidence - initiateDispute(), party1Evidence() and party2Evidence(). I have removed the latter two since they are both doing the same thing as initiateDispute(), but just wanted to see if you are ok with this change or would like it the other way around or something. You are probably referring to functions party1EvidenceURL() and party2EvidenceURL() which just serve as getters for the evidence URL - these should be ok since we will still need them.

[solomondefi-dev]

The three functions are still necessary, they're not doing the exact same thing:

    /// Initiate escrow dispute dispute
    /// @param _evidenceURL Link to real-world dispute evidence
    function initiateDispute(string memory _evidenceURL) external {
        require(msg.sender == _party1 || msg.sender == _party2, "Only parties can dispute");
        super._initiateDispute();
        if(msg.sender == _party1) {
            super._party1Evidence(_evidenceURL);
        } else {
            super._party2Evidence(_evidenceURL);
        }
    }

    /// First party dispute evidence
    /// @param _evidenceURL Link to real-world evidence
    function party1Evidence(string memory _evidenceURL) external {
        super._party1Evidence(_evidenceURL);
    }

    /// Second party dispute evidence
    /// @param _evidenceURL Link to real-world evidence
    function party2Evidence(string memory _evidenceURL) external {
        super._party2Evidence(_evidenceURL);
    }

We need them still, because if party1 calls initiateDispute, party2 still needs to call party2Evidence. The super_initiateDispute() call does this:

state = TransactionState.VotePending;
emit DisputeInitiated(_party1, _party2);
disputeTime = block.timestamp;

So it's not ok to call that twice

[jtse0]

Oh right. Gotcha. In that case I think I will need to add more handling to ensure that the initialize doesn't get called twice by both parties for the same dispute. Thanks for the clarification.

[solomondefi-dev]

You're right, we want to ensure that initiateDispute only gets called once per contract.