WIP: build boson-kleros integration poc #488
Conversation
|
|
0xartem
changed the title
@0xartem Since the Miro board is not public (I don't have access to it even with my Redeemeum account) would you mind posting a picture of it here and also summarize the approach taken by these changes? It would be nice to have a big picture understanding of what's going on before reviewing the PR. |
|
@cliffhall I am working on the BPIP. I will add the pictures there and a link to this PR. |
| bytes32 _sigS, | ||
| uint8 _sigV | ||
| ) external override disputesNotPaused nonReentrant { | ||
| function resolveDispute(uint256 _exchangeId, uint256 _percent) external override disputesNotPaused nonReentrant { |
There was a problem hiding this comment.
As mentioned in the BPIP discussion, changing this method:
- represents a breaking change that would require a major version rev to 3.0.0
- discards the signature, which is required to ensure that the two parties are in accord.
It could be added as an alternative method with a name like resolveDisputeWithProposal or something, but it's not clear why this is even part of this proposed implementation. The BPIP is about using Kleros to handle escalations. This comes prior to escalation and is not a prerequisite for such.
There was a problem hiding this comment.
Thanks for the comment @cliffhall. I am going to update the BPIP so we don't introduce the breaking change.
A new method is needed because Kleros requires two options to choose from. It means that escalation only makes sense when a buyer and a seller have proposals that don't match, like 50% and 70%.
A state of those proposals must be stored somewhere before the buyer escalates it to a DR.
| @@ -193,6 +193,9 @@ contract BosonTypes { | |||
| struct Dispute { | |||
| uint256 exchangeId; | |||
| uint256 buyerPercent; | |||
| uint256 sellerPercent; | |||
There was a problem hiding this comment.
Changing the Dispute struct is dangerous. There are implications for existing storage.
- The new fields are not at the end of the struct, and so previously stored structs will be misinterpreted and/or clobbered when written to.
- Even if they were at the end of the struct, it isn't always safe to add members to structs, depending upon how those structs are stored, i.e., in arrays vs mappings.
For these reasons, we would more likely want to associate these new fields with the dispute via a mapping and not change the struct and all the client code (including the graph) that relies on its current shape.
There was a problem hiding this comment.
Noted, will change it in the new PR.
| bytes32 _sigS, | ||
| uint8 _sigV | ||
| ) external; | ||
| function resolveDispute(uint256 _exchangeId, uint256 _percent) external; |
There was a problem hiding this comment.
See objections to this below on the DisputeHandlerFacet.
There was a problem hiding this comment.
Added changes and Rationale to the BPIP
| */ | ||
| function rule(uint256 _disputeID, uint256 _ruling) external { | ||
| BosonCase memory bosonCase = klerosDisputeCases[_disputeID]; | ||
| if (bosonCase.exchangeId == 0) revert InvalidExchangeError(); |
There was a problem hiding this comment.
Is the emission of an Error required by Kleros? Otherwise this would need to be implemented as a revert reason. We discussed implementing formal Error types but we were too far along the development track to do that before deploying V2, so we're kind of stuck with the style.
We definitely shouldn't mix and match unless there's an absolute requirement.
That's said, it's in a client, which is a boundary contract that could conceivably be given a pass if there's no other option.
There was a problem hiding this comment.
There is no such requirement from Kleros. It can be modified to match Boson coding style.
This is a draft for integration between kleros and boson protocol. The code is for information purposes and should NOT be merged directly to the main.
The following miro board contains some diagrams to inform the integration exploration.
https://miro.com/app/board/uXjVPet6BvQ=/