MultiPartyEscrow contract #39

astroseger · 2018-09-28T14:41:55Z

MultiPartEscrow contract. Unit tests for MultiPartEscrow contract. js interface for this contract for sender and recipient (in test/sign_mpe_funs.js).

contracts/MultiPartyEscrow.sol

vsbogd · 2018-10-01T17:31:03Z

contracts/MultiPartyEscrow.sol

+        require(amount <= channel.value);
+        require(msg.sender == channel.recipient);
+
+        //"this" will be added later 


Could you please explain this comment further.

vsbogd · 2018-10-01T17:36:25Z

migrations/3_MultiPartyEscrow.js

@@ -0,0 +1,12 @@
+let test1 = artifacts.require("./MultiPartyEscrow.sol");


I would propose renaming it to something verbose.

astroseger · 2018-10-02T15:27:26Z

I will merge it tomorrow, if nobody is against it.

vsbogd · 2018-10-02T15:41:19Z

Looks good for me.

tiero · 2018-10-02T22:27:20Z

contracts/MultiPartyEscrow.sol

+
+contract MultiPartyEscrow {
+
+    //it seems we don't need SafeMath


Yes, we need. When using the SafeMath library, an error will be thrown stopping the execution of the contract and it conforms to the existent codebase.

Using SafeMath for MultiPartyEscrow is a big question. I will open an Issue and explain why. And I don't understand your last part. If there is an error in the contract it must revert, there is not other possible behavior.... Ok we continue in an issue

What do you mean "codebase"? If it is signet/platform-contract then I am the first who consider using it (please make "grep SafeMath contracts/*"). And please use issue #45 for this discussion...

And Marco have you read what I've wrote in #45 ? That we are already checking for it, But still I'm hesitating to use SafeMax ? What's the point to write it here?

tiero · 2018-10-02T22:29:22Z

contracts/MultiPartyEscrow.sol

+    struct PaymentChannel {
+        address sender;      // The account sending payments.
+        address recipient;    // The account receiving the payments.
+        uint256 replica_id;  // id of particular service replica


Code Style: I think we should adhere to the existent codebase (camelCase) and in general keep code style changes as separated PRs if needed.

We should define the style... (I will follow it, even though I don't like camelCase :) ). I will open an Issue.

Yeah, I mean it's not about personal preference. If we decide to refactor the style, let's do it in a separate merge request.

As a "golden" rule, we should follow closely the master practices/styles which already exists, to make it consistent.

@tiero, do you mean some common Solidity code style practices? If so could you please share link to it at #43.

Nope, I mean the code already merged in master is the style to follow. Always.

It's better to change the majority of old code to adapt to this PR or make sure this looks consistent with the already merged code? ;)

If @pennachin wants to change the code style and he allows to spend time on that ok to me to work on #43, otherwise let's fix only this PR to match the codebase and move on.

Please define codestyle in #43 . I cannot fix codestyle without definition of codestyle... What exactly? Only names of public function or what? All those discussions are starting really pulling me backward..

Wait, there are two separate tasks here:

Merge this pull request

Define code-style

The 1 cannot go through here, because we want a consistent code style. This PR has a different style introduced in naming variables/functions like this (name_of_var) instead of the existent nameOfVar camelCase style.

The 2 came out since (maybe) we want to change and write in stone the rules (in order to setup a solidity linter/formatter in CI I presume)

The 1 and 2 can go in parallel, for the 1 the most obvious and quicker solution to me would adhere to the existent codebase, and not the other way. Moreover, I do think it's better to isolate code style changes in a specific PR.

@tiero , am I right that converting functions and variables names to camelCase will be enough for this PR?

And camelCase is the "official" style advocated by the Solidity docs.

contracts/MultiPartyEscrow.sol

tiero · 2018-10-02T22:37:50Z

contracts/MultiPartyEscrow.sol

+    }
+
+
+    function isValidSignature_open_channel_replaysafe(address recipient, uint256 value, uint256 expiration, uint256 replica_id, uint256 message_nonce, bytes memory signature, address sender)


I would decouple the logic of isValidSiganture and move the business logic where is used instead of having a specific function. They are both used once in the whole code, useless as specific helpers

agree! And it make contract a little bit cheaper :) (35884 vs 36027 ) ... Will commit this evening. In airport now.

contracts/MultiPartyEscrow.sol

tiero · 2018-10-02T22:46:30Z

contracts/MultiPartyEscrow.sol

+    {
+        require(msg.sender == channels[channel_id].sender);
+        require(now >= channels[channel_id].expiration);
+        _channel_sendback_and_reopen(channel_id);


It seems strange that we claim the timeout, mostly because the service provider is not reachable or trying to cheat, and we want to open back a channel. Should not be only closed?

One of the main ideas of such design is that we can have single channel for each client/service provider pair and we don't have to close it. When one of the parties gets money then channel none is incremented and channel can be used again. @astroseger please correct me if I am wrong.

Makes sense, but you agree that in this situation, most of the time the sender will not do business again with this recipient. Ok for me to leave the feature but at least we should emit an event at least. It's very important to keep track of this case, for reputation especially cc/ @pennachin (metaphorically, you are opting out your stake from the service provider)

Marco! It is the name of function which is confusing you. I will rename it to _channel_sendback_and_suspend . Because it reopen the channel but change value and expiration to zero. So in order to start to use it the client should put money in it.

but it is actually reopen ... so I will call the function _channel_sendback_and_reopen_suspended

…ture* functions

astroseger · 2018-10-03T21:07:36Z

I've opened several issues and I've made several changes... It seems all questions have been answered.

astroseger · 2018-10-04T10:30:17Z

Ok guys... I will do the following steps

I will remove open_channel_by_recipient logic for now, and will create an issue with discussion. We are not sure that this functionality will be adopted. So if we decide to adopt it, I will add it again.
I will change the style (but Unfortunately it makes the code much less readable _channelSendbackAndReopenSuspended )
I will merge
If you'are against item 3. Please scream, but please think twice :)

astroseger added 2 commits September 28, 2018 17:35

MultiPartyEscrow contract

6e72b8c

add ethereumjs-abi and ethereumjs-util into package.json

161045f

astroseger requested review from tiero and vsbogd September 28, 2018 15:10

tiero requested a review from vforvalerio87 September 28, 2018 15:36

add comment that nonce prevents race condtions

cd0950d

vsbogd reviewed Oct 1, 2018

View reviewed changes

contracts/MultiPartyEscrow.sol Outdated Show resolved Hide resolved

vsbogd reviewed Oct 1, 2018

View reviewed changes

contracts/MultiPartyEscrow.sol Outdated Show resolved Hide resolved

vsbogd reviewed Oct 1, 2018

View reviewed changes

contracts/MultiPartyEscrow.sol Show resolved Hide resolved

vsbogd reviewed Oct 1, 2018

View reviewed changes

few comments and variable renaming

9978042

tiero requested changes Oct 2, 2018

View reviewed changes

rename _channel_sendback_and_reopen_suspended and remove isValidSigna…

31c416f

…ture* functions

vsbogd mentioned this pull request Oct 4, 2018

Add gRPC interceptor stub to check gRPC escrow contract payment channel state singnet/snet-daemon#54

Merged

astroseger added 2 commits October 4, 2018 13:52

changeToThisUglyFormat

52ca590

remove openChannelByRecipient logic

fff3de3

vsbogd approved these changes Oct 4, 2018

View reviewed changes

tiero approved these changes Oct 4, 2018

View reviewed changes

astroseger merged commit 2e52566 into singnet:master Oct 4, 2018

astroseger mentioned this pull request Oct 4, 2018

Multi-Party Escrow #30

Closed

astroseger deleted the develop-multiparty-escrow-contracts branch October 4, 2018 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MultiPartyEscrow contract #39

MultiPartyEscrow contract #39

astroseger commented Sep 28, 2018

vsbogd Oct 1, 2018

vsbogd Oct 1, 2018

astroseger commented Oct 2, 2018

vsbogd commented Oct 2, 2018

tiero Oct 2, 2018

astroseger Oct 3, 2018

tiero Oct 4, 2018 •

edited

Loading

astroseger Oct 4, 2018

astroseger Oct 4, 2018 •

edited

Loading

tiero Oct 2, 2018

astroseger Oct 3, 2018

tiero Oct 3, 2018

vsbogd Oct 4, 2018

tiero Oct 4, 2018 •

edited

Loading

astroseger Oct 4, 2018 •

edited

Loading

tiero Oct 4, 2018

vsbogd Oct 4, 2018 •

edited

Loading

tiero Oct 4, 2018

pennachin Oct 4, 2018

tiero Oct 2, 2018

astroseger Oct 3, 2018

tiero Oct 2, 2018

vsbogd Oct 3, 2018

tiero Oct 3, 2018

astroseger Oct 3, 2018 •

edited

Loading

astroseger Oct 3, 2018

tiero Oct 3, 2018

astroseger commented Oct 3, 2018 •

edited

Loading

astroseger commented Oct 4, 2018 •

edited

Loading

		@@ -0,0 +1,12 @@
		let test1 = artifacts.require("./MultiPartyEscrow.sol");


		contract MultiPartyEscrow {

		//it seems we don't need SafeMath

		}


		function isValidSignature_open_channel_replaysafe(address recipient, uint256 value, uint256 expiration, uint256 replica_id, uint256 message_nonce, bytes memory signature, address sender)

MultiPartyEscrow contract #39

MultiPartyEscrow contract #39

Conversation

astroseger commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astroseger commented Oct 2, 2018

vsbogd commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiero Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astroseger Oct 4, 2018 • edited Loading

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

tiero Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

astroseger Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsbogd Oct 4, 2018 • edited Loading

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

astroseger Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astroseger commented Oct 3, 2018 • edited Loading

astroseger commented Oct 4, 2018 • edited Loading

tiero Oct 4, 2018 •

edited

Loading

astroseger Oct 4, 2018 •

edited

Loading

tiero Oct 4, 2018 •

edited

Loading

astroseger Oct 4, 2018 •

edited

Loading

vsbogd Oct 4, 2018 •

edited

Loading

astroseger Oct 3, 2018 •

edited

Loading

astroseger commented Oct 3, 2018 •

edited

Loading

astroseger commented Oct 4, 2018 •

edited

Loading