swap: replaced Owner for Issuer #1720

santicomp2014 · 2019-09-02T14:50:19Z

Resolves Issue #1553
Changes Owner for Issuer, So the terminology is the same as the Smart Contract.

mortelli · 2019-09-02T14:53:46Z

swap/cheque.go

@@ -131,7 +131,7 @@ func (cheque *Cheque) verifyChequeProperties(p *Peer, expectedBeneficiary common
 		return fmt.Errorf("wrong cheque parameters: expected contract: %x, was: %x", p.contractAddress, cheque.Contract)
 	}

-	// the beneficiary is the owner of the counterparty swap contract
+	// the beneficiary is the issuer of the counterparty swap contract


since a contract is not issued (unlike a cheque) i don't think the terminology change should be applied in this case

the contract does not use the owner terminology anymore. But you are correct that the contract is not issued (the "issuer" is the party allowed to issue cheques but not the "issuer" of the contract). allowed issuer or issuing party might be some option, but I think outside the context of a cheque we can also keep it as owner on the go side.

We can also consider cheque issuer. I would not use the terminology owner anymore, to avoid confusion.

Apart from that, I think this comment is somewhat confusing whatever naming we use, as in SWAP a beneficiary can at one point also be issuer (as is the case here). I would propose:

p.beneficiary (to emphasize that this peer is not the beneficiary of the cheque) is the issuer of cheques of the counterparty swap contract.

Ideally, I would like peer.beneficiary to be peer.counterpary, as a counterparty can be both a beneficiary and an issuer. Right now, it looks as if a beneficiary becomes an issuer and that it is very confusing.

If we do that, the comment can just be the counterparty must have signed.

I will commit the changed comment, the beneficiary is the issuer of cheques of the counterparty swap contract.

I replaced all instances just to be on the safe side, but I agree it depends on the context.
I agree that it's a bit confusing.

mortelli · 2019-09-02T14:58:29Z

swap/swap_test.go

-// the owner of this swap is the beneficiaryAddress
-// hence the owner of this swap would sign cheques with beneficiaryKey and receive cheques from ownerKey (or another party) which is NOT the owner of this swap
+// the issuer of this swap is the beneficiaryAddress
+// hence the issuer of this swap would sign cheques with beneficiaryKey and receive cheques from issuerKey (or another party) which is NOT the issuer of this swap


same as above: not convinced swap or contract can use the terminology of issued like cheques can

The confusion here has in my opinion not so much to do with renaming owner to issuer.

It underlines, IMO, that we have created somewhat of confusin situation due to naming:

Here we see again that an issuer can be also a beneficiary and vice versa. What makes it extra confusing here, is that in the Swarm source code (protocol.go:124 and peer.go:34), we speak about the peer.beneficiary as the counterparty and we set our own address and contract in the swap struct. Whereas here, we are just doing it the other way around, saving the issuer in the peer struct and the beneficiary in the swap struct.

We have 4 parties to name properly:

Counterparty Externally Owned account,
Currently peer.beneficiary

Counterparty chequebook address,
Currently peer.contractAddress

Our own Externally Owned account,
Currently swap.issuer.address

Our own chequebook address
Currently swap.issuer.Contract

What do you think: is it worth spending time on attempting to find perfect names for those, or shall we just leave as it is, and accept that there can be ambiguity and that we risk making mistakes as in the function newTestSwapAndPeer?

I understand the cause of confusion, should I leave alone for now?.

Or should I refactor the comments?, I'm a bit confused.

I would leave this one for now. I will bring it up as a point on the agenda for our next sync meeting!

mortelli

i am leaving a general comment here in order not to spam, but my overall impression is that we shouldn't use the term issuer when talking about either the Swap structure or making direct references to contracts (instead of cheques).

i am aware that issue #1553 does not state this, but i am objecting to it; to me, owner still feels better suited to these cases.

tagging @Eknir as well as @ralph-pichler since they are the ones most familiar with the terminology used in the SCs. do they use the term owner at all?

i will approve the PR if we are not in agreement on this and a case can be made for the opposing view 👍

ralph-pichler

"Simplest" Swap has landed on master. Please rebase.

Eknir

Good first PR and sorry for the long comments!

I generally have the feeling, as expressed in the comments, that our naming is not always spot on and that this might create confusion. I remember though the wise words from @mortelli (ask him about this) and perhaps we should just move on and don't attempt to make the naming better.

Nevertheless, I think it is an improvement to rename issuer to owner, as owner is not used anymore on the smart-contract side.

I approve the PR, as I think it is good, as is. We could perhaps make it better, but then I would like to assess the whole issue with regards to naming and not only talk about renaming owner to issuer.

Eknir · 2019-09-03T07:56:01Z

swap/cheque.go

@@ -131,7 +131,7 @@ func (cheque *Cheque) verifyChequeProperties(p *Peer, expectedBeneficiary common
 		return fmt.Errorf("wrong cheque parameters: expected contract: %x, was: %x", p.contractAddress, cheque.Contract)
 	}

-	// the beneficiary is the owner of the counterparty swap contract
+	// the beneficiary is the issuer of the counterparty swap contract


We can also consider cheque issuer. I would not use the terminology owner anymore, to avoid confusion.

Apart from that, I think this comment is somewhat confusing whatever naming we use, as in SWAP a beneficiary can at one point also be issuer (as is the case here). I would propose:

p.beneficiary (to emphasize that this peer is not the beneficiary of the cheque) is the issuer of cheques of the counterparty swap contract.

Ideally, I would like peer.beneficiary to be peer.counterpary, as a counterparty can be both a beneficiary and an issuer. Right now, it looks as if a beneficiary becomes an issuer and that it is very confusing.

If we do that, the comment can just be the counterparty must have signed.

Eknir · 2019-09-03T08:15:39Z

swap/swap.go

-// getContractOwner retrieve the owner of the chequebook at address from the blockchain
-func (s *Swap) getContractOwner(ctx context.Context, address common.Address) (common.Address, error) {
+// getContractIssuer retrieve the issuer of the chequebook at address from the blockchain
+func (s *Swap) getContractIssuer(ctx context.Context, address common.Address) (common.Address, error) {


You could make it: getIssuerAtContract, or decide that we can cope with some ambiguity here.

It makes more sense this way.

Eknir · 2019-09-03T08:49:15Z

swap/swap_test.go

-// the owner of this swap is the beneficiaryAddress
-// hence the owner of this swap would sign cheques with beneficiaryKey and receive cheques from ownerKey (or another party) which is NOT the owner of this swap
+// the issuer of this swap is the beneficiaryAddress
+// hence the issuer of this swap would sign cheques with beneficiaryKey and receive cheques from issuerKey (or another party) which is NOT the issuer of this swap


The confusion here has in my opinion not so much to do with renaming owner to issuer.

It underlines, IMO, that we have created somewhat of confusin situation due to naming:

Here we see again that an issuer can be also a beneficiary and vice versa. What makes it extra confusing here, is that in the Swarm source code (protocol.go:124 and peer.go:34), we speak about the peer.beneficiary as the counterparty and we set our own address and contract in the swap struct. Whereas here, we are just doing it the other way around, saving the issuer in the peer struct and the beneficiary in the swap struct.

We have 4 parties to name properly:

Counterparty Externally Owned account,
Currently peer.beneficiary

Counterparty chequebook address,
Currently peer.contractAddress

Our own Externally Owned account,
Currently swap.issuer.address

Our own chequebook address
Currently swap.issuer.Contract

What do you think: is it worth spending time on attempting to find perfect names for those, or shall we just leave as it is, and accept that there can be ambiguity and that we risk making mistakes as in the function newTestSwapAndPeer?

…ambiguity

holisticode · 2019-09-03T20:33:25Z

swap/protocol.go

@@ -105,7 +105,7 @@ func (s *Swap) run(p *p2p.Peer, rw p2p.MsgReadWriter) error {
 	protoPeer := protocols.NewPeer(p, rw, Spec)

 	handshake, err := protoPeer.Handshake(context.Background(), &HandshakeMsg{
-		ContractAddress: s.owner.Contract,
+		ContractAddress: s.issuer.Contract,


This is exactly what I meant this morning in the call.

This is the handshake. At this point, two nodes are just exchanging contract addresses. We don't even know what balances there are (they might be 0 on both sides at this point!). Thus, I see it as incorrect to change this to issuer. No cheques may have been issued at this point, thus no issuer exists. Furthermore, at this point both nodes could be the issuer of cheques! I think it does not make sense to introduce this change here.

i agree.

if we don't want to use owner for reasons of consistency with the smart contracts, we can find another word.

in any case, the golang context is different from the SC context, so it wouldn't bother me if we were to use additional terms on either side that the other side doesn't.

i'd just insist on using the same terms when referring to the same entities, specially in variables or fields.

mortelli

i have to say that at this point, i am not sure of how to move forward here.

i cannot approve this PR, nor request changes, since it seems many of us have different opinions on what each thing should be named.

i am officially requesting commentary/thoughts on how to proceed in this case: from @holisticode due to being the track lead, and/or @vojtechsimetka due to being the issue creator.

mortelli · 2019-09-04T17:22:36Z

swap/cheque.go

@@ -116,7 +116,7 @@ func (cheque *Cheque) verifyChequeProperties(p *Peer, expectedBeneficiary common
 		return fmt.Errorf("wrong cheque parameters: expected contract: %x, was: %x", p.contractAddress, cheque.Contract)
 	}

-	// the beneficiary is the owner of the counterparty swap contract
+	// the beneficiary is the issuer of cheques of the counterparty swap contract.
 	if err := cheque.VerifySig(p.beneficiary); err != nil {


so if i understand this correctly, the mentioned counterparty here would be the peer p, right?

i find it confusing to call this beneficiary, as it could be interpreted as the address this peer writes cheques to or the address this peer wants its cheques written to just as well.

also: remove . from this comment

Agree about beneficiary being confusing. I would change it to p.counterparty

mortelli · 2019-09-04T17:24:53Z

swap/protocol.go

@@ -105,7 +105,7 @@ func (s *Swap) run(p *p2p.Peer, rw p2p.MsgReadWriter) error {
 	protoPeer := protocols.NewPeer(p, rw, Spec)

 	handshake, err := protoPeer.Handshake(context.Background(), &HandshakeMsg{
-		ContractAddress: s.owner.Contract,
+		ContractAddress: s.issuer.Contract,


i agree.

if we don't want to use owner for reasons of consistency with the smart contracts, we can find another word.

in any case, the golang context is different from the SC context, so it wouldn't bother me if we were to use additional terms on either side that the other side doesn't.

i'd just insist on using the same terms when referring to the same entities, specially in variables or fields.

mortelli · 2019-09-04T17:26:20Z

swap/protocol_test.go

@@ -47,7 +47,7 @@ func TestHandshake(t *testing.T) {
 		t.Fatal(err)
 	}
 	// setup the protocolTester, which will allow protocol testing by sending messages
-	protocolTester := p2ptest.NewProtocolTester(swap.owner.privateKey, 2, swap.run)


same here, as commented above (i will refrain from repeating it from here onwards)

santicomp2014 · 2019-09-10T15:12:09Z

Owner to Issuer is not agreed on by consensus.
So for the time being this will be on hold until a better nomenclature comes along.

Eknir · 2019-10-21T07:51:04Z

Can we close this PR and address the proposed changes here in the upcoming rename PR?
@santicomp2014

santicomp2014 · 2019-12-16T15:32:57Z

This PR was closed due to the outdated changes between this code and the latest additions of ERC20 and Swap Factory.

It makes more sense to branch of master, and then include the comments that were provided in this PR.

replaced Owner for Issuer

82aa9e3

santicomp2014 added the incentives label Sep 2, 2019

santicomp2014 requested review from mortelli and Eknir September 2, 2019 14:50

mortelli reviewed Sep 2, 2019

View reviewed changes

mortelli suggested changes Sep 2, 2019

View reviewed changes

santicomp2014 requested a review from mortelli September 2, 2019 16:58

fix whitespace

9c7a0ee

ralph-pichler reviewed Sep 2, 2019

View reviewed changes

Eknir approved these changes Sep 3, 2019

View reviewed changes

santicomp2014 changed the title ~~replaced Owner for Issuer~~ swap: replaced Owner for Issuer Sep 3, 2019

santicomp2014 added 3 commits September 3, 2019 12:27

swap: merge from master

4694a7c

swap: cheque issuer comment fixed

5331f06

swap: changed method getContractIssuer for getIssuerAtContract, less …

4d43225

…ambiguity

santicomp2014 requested a review from holisticode September 3, 2019 18:38

holisticode reviewed Sep 3, 2019

View reviewed changes

mortelli reviewed Sep 4, 2019

View reviewed changes

santicomp2014 added the on hold label Sep 10, 2019

mortelli assigned santicomp2014 Nov 4, 2019

santicomp2014 closed this Dec 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swap: replaced Owner for Issuer #1720

swap: replaced Owner for Issuer #1720

santicomp2014 commented Sep 2, 2019

mortelli Sep 2, 2019

ralph-pichler Sep 2, 2019

Eknir Sep 3, 2019

santicomp2014 Sep 3, 2019

mortelli Sep 2, 2019

Eknir Sep 3, 2019

santicomp2014 Sep 3, 2019

Eknir Sep 4, 2019

mortelli left a comment •

edited

Loading

ralph-pichler left a comment

Eknir left a comment

Eknir Sep 3, 2019

Eknir Sep 3, 2019

santicomp2014 Sep 3, 2019

Eknir Sep 3, 2019

holisticode Sep 3, 2019

mortelli Sep 4, 2019

mortelli left a comment

mortelli Sep 4, 2019

Eknir Sep 5, 2019 •

edited

Loading

mortelli Sep 4, 2019

mortelli Sep 4, 2019

santicomp2014 commented Sep 10, 2019

Eknir commented Oct 21, 2019 •

edited

Loading

santicomp2014 commented Dec 16, 2019

swap: replaced Owner for Issuer #1720

swap: replaced Owner for Issuer #1720

Conversation

santicomp2014 commented Sep 2, 2019

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

mortelli left a comment • edited Loading

Choose a reason for hiding this comment

ralph-pichler left a comment

Choose a reason for hiding this comment

Eknir 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

mortelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eknir Sep 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santicomp2014 commented Sep 10, 2019

Eknir commented Oct 21, 2019 • edited Loading

santicomp2014 commented Dec 16, 2019

mortelli left a comment •

edited

Loading

Eknir Sep 5, 2019 •

edited

Loading

Eknir commented Oct 21, 2019 •

edited

Loading