cryptonetlab / onchain-storage

Onchain.Storage is an ecosystem of interoperable storage protocols.
https://onchain.storage
5 stars 1 forks source link

Batch operator Smart Contract #8

Closed turinglabsorg closed 1 year ago

turinglabsorg commented 1 year ago

This issue will be used to track and discuss about how we want to manage bulk operations inside our smart contracts, the output of the discussion will be this design doc on hackmd.

Estimated time

4 weeks

Effort

Write design docs, find an agreement on the implementation and build both Smart Contract and UI implementation.

cc @irenegia @nicola

irenegia commented 1 year ago

@turinglabsorg the link doesn't work for me

turinglabsorg commented 1 year ago

@irenegia check now, it wasn't published yet

turinglabsorg commented 1 year ago

@irenegia @nicola I'm doing some deep researches and i found some blockers.

First blocker

While creating multiple reading calls is very simple (because we're reading the state), change the state by aggregating transactions is a little bit more tricky because we have to handle with msg.sender and tx.origin.

We basically have two ways to call a contract B (protocol contract) from a contract A (bulk operator):

This means that we can't batch transactions using CALL because otherwise the deals will be done with the contract, not the first user, and we can't use DELEGATECALL because doesn't changes the state in B contract.

If we want to use CALL we should change all the protocols by adding another parameter to all the functions by specifying from who the transaction is originated like:

function createDealProposal(
        address _dealer,
        string memory _data_uri,
        uint256 duration,
        uint256 collateral,
        address[] memory _providers,
        address[] memory _appeal_addresses
    )

by adding the _dealer parameter we can allow other addresses (contracts or also other operators) create deals for other addresses. This can be an interesting improvement for all the contracts but we have to agree on that, because it will impact the expected time.

Second blocker

Since we're creating a modular ecosystem we should agree on the way we want to implement future protocols (because each protocol can implement its own interface):

Since we're creating a proxy contract which will proxy the call to other contracts (and those contracts can be upgradeable as well) i would suggest to reduce complexity and handle with contract address change.

irenegia commented 1 year ago

About the first issue:

we can't use DELEGATECALL because doesn't changes the state in B contract.

in our case this means that contract B (eg, the retriev contract) will not record that deals are proposed/accepted. Correct?

adding another parameter to all the functions by specifying from who the transaction is originated like

okay, but now anyone could create a deal proposal on my behalf? And do we lock tokens for the payment from which addresses?

irenegia commented 1 year ago

I suggest that we create a separate issue to discuss the second blocker. It is orthogonal to bulk opration, we need to decide in general how to deal with protocol major re-designs

turinglabsorg commented 1 year ago

About the first issue:

we can't use DELEGATECALL because doesn't changes the state in B contract.

in our case this means that contract B (eg, the retriev contract) will not record that deals are proposed/accepted. Correct?

Yes, this is the issue basically. It's a little bit more complicated, if we use delegatecall we need the same storage on contract A, because the storage of the context is the A one, not the B one. This means we need to add all storages of all the protocols, be sure we don't have overlaps..

adding another parameter to all the functions by specifying from who the transaction is originated like

okay, but now anyone could create a deal proposal on my behalf? And do we lock tokens for the payment from which addresses?

Yes exactly, an user A can create a deal for the user B. In this case (actual implementation) money are taken by the user A which creates the transaction. In case of refund, appeal etc it will be the user B to interact with protocol (as he actually invoked the protocol).

turinglabsorg commented 1 year ago

@irenegia @lucaniz adding three screenshot of the cases descripted above, leave comments if something needs to be clarified, sharing also the Miro board if needed for some reason.

image

image

image

turinglabsorg commented 1 year ago

@irenegia to go ahead with this task I need a final 👍 regarding the change on Retriev protocol. Since only the design is needed for current sprint (28th Nov - 9th Dec) i think we have enough time to talk about that on next meeting.

cc @nicola @lucaniz

irenegia commented 1 year ago

@turinglabsorg sorry, I got lost here: which is the chosen design? A, B1 or B2 ?

Also, I left some comments in the HackMD!

turinglabsorg commented 1 year ago

If we want to implement we have to choose B, otherwise it won't work.

B1 and B2 are different ways to use same contract!

Will check comments later today ;-)

irenegia commented 1 year ago

If we want to implement we have to choose B, otherwise it won't work.

You mean A?

turinglabsorg commented 1 year ago

No, A is "leave protocol as it is". If we leave the protocol as it is the owner of the deal will be the bulk contract, not the original user.

irenegia commented 1 year ago

okay, so the suggestion is to trust the bulk contract to add the dealer (maybe call it owner to use the same name we used in the new createDeal function https://github.com/protocol/retriev/issues/91# ?) param. Right?

Is there something that can go wrong here? Like, can this be abused to make a deal on behalf of an unaware client? is this case B2?

turinglabsorg commented 1 year ago

Yes, proposal is to add the same parameter as the createdeal new function, we can lock the possibility to add a different owner only to a specific contract so case B2 is not possible.

Again the worst thing can happen is spam, but only if we don't lock the feature.

We don't need to trust the contract in any case, because we can automatically write the field owner without leaving the possibility to change it for the end user.

Maybe it's easier if I write some abstract code to explain the flow?

irenegia commented 1 year ago

we can lock the possibility to add a different owner only to a specific contract

sorry, I don't fully get this

turinglabsorg commented 1 year ago

@irenegia sorry I was on mobile and I didn't wrote the comment well. I tried to summarize the problem in this doc. Let me know if now it's clear enough, we eventually use the doc as a draft for this change proposal to have a full 👍 from the team!

cc @lucaniz

irenegia commented 1 year ago

read the doc, left a comment there!

irenegia commented 1 year ago

So, the client sends one transaction with multiple "instructions" to bulk sm and the bulk does all the call following the instructions. Correct?

turinglabsorg commented 1 year ago

Closing since design was successfully completed.