feat: market crud #1565

quasisamurai · 2018-10-01T12:58:50Z

resolved problem

Here is yet not finished Proof-of-concept CRUD for Market. While I worked on this, i found some critical issues caused by Solidity & EVM design. So let's start:

I will describe problem using orders, but it's the same for deals. So when we compare two orders (for ex. in OpenDeal) now we're using this construction. Notice, that Order is a struct.
Order memory ask = orders[_askID]; Order memory bid = orders[_bidID]
This one reffers only for 4 variables, named

ask,bid, _askID, _bidID.

But when you're trying to implement CRUD, you need to get exact vars from storage SC.
There are two noticable things aproaching then:

you should use multiple return due solidity currently not supporting struct returning (only in internal functions, that's not applicable for crud)
declare every variable and cast a returned value of getter into concrete type, due contract's functions operate with tuples in case of multiple return.

So data taken from storage contract woul'd be look like this:
( Orders.OrderType orderType, address author, address counterparty, uint duration, uint price, bool[] memory netflags, ProfileRegistry.IdentityLevel identityLevel, address blacklist, bytes32 tag, uint64[] memory benchmarks, uint frozenSum) = ord.GetOrderInfo(askID); Here is the part of order, the immutable data.

This situation totally rapes compiler by calling only a part of one order. Even without casting this vars as a struct But we need two full orders. And that's is only begining.

Why this is going on like this?
Solidity has a limit for local variables (16).

Depending on what you do, you can have around 16 local variables (including parameters and return parameters)

So every input parameter to the function takes one variable, and each return value takes one variable, and each struct cast, and each local declaration takes another variable, and references to storage take two - it surprised me how quickly the limit of 16 was reached.

struct a = struct(b,c,d)

will reserve 5 slots in stack (i'm not sure, it's just an empirical conclusion)
That is an old problem, we faced some months ago and it was a cause why we splitted one order/deal getter by two for both structs.

Since almost each function uses one/two orders or a deal sctruct, there is no way to implement crud this way. That's not the only issue i faced, but I think this is the most significant problem that needs to be solved.

So here is implemented crud for market
Included:

Deals
Orders
ChangeRequests
Master-worker contract named Administratum (discussable)
Also crud for Administratum
Updated event WorkerConfirmed (api-breaking)
Updated test enviroment (watch truffle.js & test.sh)
Aslo fixed some non-critical bugs

By the way we faced and original problem binded with this EIP. So we decided to modify our stack to avoid problems with contract's bytecode limit.

p.s. some weird comments and getters will be removed after refactoring iteration soon. 🇺🇦

sokel · 2018-11-01T10:17:03Z

blockchain/source/test/market.js

-                await market.RemoveWorker(accounts[0], master, { from: master });
-            });
-        });
+        // describe('RegisterWorker', () => {


any reason for hide this tests?

due this section tests cases while market is paused. Now master-worker relations is separated contract. Later, me or @abefimov will update tests and may be administratum would be tested in separate test file. And i leaved (just commented, not deleted) this tests for the future.

keep tests clean.
Remove old and write new.
Don't do it like that.

blockchain/source/contracts/Market.sol

blockchain/source/contracts/Deals.sol

blockchain/source/contracts/Market.sol

antmat

still reviewing

blockchain/source/contracts/Administratum.sol

blockchain/source/contracts/ChangeRequests.sol

blockchain/source/contracts/Deals.sol

blockchain/source/contracts/Market.sol

antmat · 2018-11-08T15:59:39Z

blockchain/source/contracts/Market.sol


-    uint dealAmount = 0;
+    Deals dl;


can we use better names here?

@quasisamurai why? Choose better name pls.

blockchain/source/contracts/Deals.sol

blockchain/source/contracts/Administratum.sol

blockchain/source/contracts/AdministratumCrud.sol

blockchain/source/contracts/Deals.sol

blockchain/source/contracts/Market.sol

blockchain/source/migrations/10_set_market_for_blacklist.js

sokel · 2018-11-12T12:15:51Z

blockchain/source/test/market.js

-                await market.RemoveWorker(accounts[0], master, { from: master });
-            });
-        });
+        // describe('RegisterWorker', () => {


keep tests clean.
Remove old and write new.
Don't do it like that.

blockchain/source/truffle.js

sokel · 2018-11-12T12:16:16Z

blockchain/source/truffle.js

@@ -58,7 +60,7 @@ module.exports = {
    },
    solc: {
        optimizer: {
-            enabled: true,
+            enabled: false,


sokel · 2018-11-20T11:18:55Z

blockchain/source/contracts/Administratum.sol

+
+
+
+    //modifiers


what is this?

sokel · 2018-11-20T11:20:29Z

blockchain/source/contracts/Administratum.sol

+import "zeppelin-solidity/contracts/ownership/Ownable.sol";
+import "./AdministratumCrud.sol";
+
+contract Administratum is Ownable {


Administratable or Ownable

sokel · 2018-11-20T11:27:15Z

blockchain/source/contracts/Market.sol


-        OpenDeal(askID, GetOrdersAmount());
+        OpenDeal(askID, ordersCrud.GetOrdersAmount());
    }

    // Deal functions

    function OpenDeal(uint _askID, uint _bidID) whenNotPaused public {


public should be first modifier

sokel · 2018-11-20T11:31:50Z

blockchain/source/contracts/Market.sol


        // if deal is normal
-        if (ask.duration != 0) {
-            endTime = startTime.add(bid.duration);
+        if (ordersCrud.GetOrderDuration(_askID) != 0) {


what is normal deal is this?
we already have spot and forward deals.

orders crud master-worker crud deals crud change requests crud massive market reworking update tests update migrations least crud contracts updations update gas usage in truffle.js contracts works fine migrations updated, will refactor later

* administratum: advanced master-worker relations * feat: change onwable => administratable * orders crud * master-worker crud * deals crud * change requests crud * massive market reworking * feat: migrate func added * crud for administratum * fix: cr status bug * fix: counterpartry bid matching * fix: migrate order * rating fields

quasisamurai added T: API breaking This PR/Issue breaks API T: WIP This PR/Issue still in progress P: medium This PR/Issue has normal priority S: Smart-contracts This PR/Issue changes SC T: experiment This PR/Issue is an experiment T: perf This PR/Issue improves performance labels Oct 1, 2018

quasisamurai requested a review from a team as a code owner October 1, 2018 12:58

quasisamurai force-pushed the feat/market-rework branch from 2cf6c00 to 7f3478e Compare October 1, 2018 13:06

quasisamurai force-pushed the feat/market-rework branch from 14fdcc2 to b83db58 Compare October 31, 2018 15:07

quasisamurai changed the title ~~discuss: market rework~~ feat: market crud Oct 31, 2018

quasisamurai force-pushed the feat/market-rework branch 2 times, most recently from d7cde6a to b9b5883 Compare November 1, 2018 09:45

sokel requested changes Nov 1, 2018

View reviewed changes

quasisamurai removed the T: WIP This PR/Issue still in progress label Nov 1, 2018

quasisamurai requested a review from antmat November 1, 2018 11:07

antmat requested changes Nov 9, 2018

View reviewed changes

sokel requested changes Nov 12, 2018

View reviewed changes

quasisamurai force-pushed the feat/market-rework branch 4 times, most recently from bbc1da5 to 2f0bcb1 Compare November 19, 2018 10:16

antmat changed the base branch from master to feat/market_v2 November 20, 2018 11:12

antmat force-pushed the feat/market-rework branch from 2f0bcb1 to 9cf59d4 Compare November 20, 2018 11:17

sokel requested changes Nov 20, 2018

View reviewed changes

quasisamurai force-pushed the feat/market-rework branch from 9cf59d4 to af19f6c Compare November 20, 2018 11:27

sokel requested changes Nov 21, 2018

View reviewed changes

quasisamurai added 4 commits November 27, 2018 17:09

tests updated

8efee2d

linted

be1654f

remove limit for da contract bytecode length

da992f4

quasisamurai added 14 commits November 27, 2018 17:09

some refactoring

1587b79

feat: migrate func added

3c85c40

some style mistakes fixed

a1771a0

crud for administratum

5e730d6

and update its migrations

cd5b559

and update its tests

d459acc

clean

e407070

fix: cr status bug

991089d

fix: counterpartry bid matching

fb870b5

fix: migrations order

b51d751

feat: change onwable => administratable

0fe57a3

rating fields commited

dc07e53

fix tuple err while generating abi

4dd15b5

optimization enabled back

806b999

quasisamurai force-pushed the feat/market-rework branch from 0b47038 to 806b999 Compare November 27, 2018 14:10

quasisamurai requested review from 3Hren, zavgorodnii, nikonov1101 and a team as code owners November 27, 2018 14:10

antmat force-pushed the feat/market-rework branch 2 times, most recently from 9cf59d4 to 806b999 Compare November 27, 2018 15:50

sokel approved these changes Nov 27, 2018

View reviewed changes

sokel merged commit 514a72c into feat/market_v2 Nov 27, 2018

sokel deleted the feat/market-rework branch November 27, 2018 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: market crud #1565

feat: market crud #1565

quasisamurai commented Oct 1, 2018 •

edited

Loading

sokel Nov 1, 2018

quasisamurai Nov 1, 2018

sokel Nov 12, 2018

antmat left a comment

antmat Nov 8, 2018

quasisamurai Nov 12, 2018

sokel Nov 12, 2018

sokel Nov 12, 2018

sokel Nov 12, 2018

quasisamurai Nov 12, 2018

sokel Nov 13, 2018

sokel Nov 20, 2018

sokel Nov 20, 2018

sokel Nov 20, 2018

sokel Nov 20, 2018

feat: market crud #1565

feat: market crud #1565

Conversation

quasisamurai commented Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antmat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quasisamurai commented Oct 1, 2018 •

edited

Loading