Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

swap: replaced Owner for Issuer #1720

Closed
wants to merge 5 commits into from
Closed

swap: replaced Owner for Issuer #1720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
82aa9e3
replaced Owner for Issuer
Aug 29, 2019
9c7a0ee
fix whitespace
Sep 2, 2019
4694a7c
swap: merge from master
santicomp2014 Sep 3, 2019
5331f06
swap: cheque issuer comment fixed
santicomp2014 Sep 3, 2019
4d43225
swap: changed method getContractIssuer for getIssuerAtContract, less …
santicomp2014 Sep 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion swap/cheque.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@Eknir Eknir Sep 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return err
}
Expand Down
4 changes: 2 additions & 2 deletions swap/protocol.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}, s.verifyHandshake)
if err != nil {
return err
Expand All @@ -116,7 +116,7 @@ func (s *Swap) run(p *p2p.Peer, rw p2p.MsgReadWriter) error {
return ErrInvalidHandshakeMsg
}

beneficiary, err := s.getContractOwner(context.Background(), response.ContractAddress)
beneficiary, err := s.getIssuerAtContract(context.Background(), response.ContractAddress)
if err != nil {
return err
}
Expand Down
22 changes: 11 additions & 11 deletions swap/protocol_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func TestHandshake(t *testing.T) {
var err error

// setup test swap object
swap, clean := newTestSwap(t, ownerKey)
swap, clean := newTestSwap(t, issuerKey)
defer clean()

ctx := context.Background()
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

protocolTester := p2ptest.NewProtocolTester(swap.issuer.privateKey, 2, swap.run)

// shortcut to creditor node
debitor := protocolTester.Nodes[0]
Expand All @@ -60,7 +60,7 @@ func TestHandshake(t *testing.T) {
cheque := newTestCheque()

// sign the cheque
cheque.Signature, err = cheque.Sign(swap.owner.privateKey)
cheque.Signature, err = cheque.Sign(swap.issuer.privateKey)
if err != nil {
t.Fatal(err)
}
Expand All @@ -83,14 +83,14 @@ func TestHandshake(t *testing.T) {
{
Code: 0,
Msg: &HandshakeMsg{
ContractAddress: swap.owner.Contract,
ContractAddress: swap.issuer.Contract,
},
Peer: creditor.ID(),
},
{
Code: 0,
Msg: &HandshakeMsg{
ContractAddress: swap.owner.Contract,
ContractAddress: swap.issuer.Contract,
},
Peer: debitor.ID(),
},
Expand All @@ -112,7 +112,7 @@ func TestHandshake(t *testing.T) {
func TestEmitCheque(t *testing.T) {
log.Debug("set up test swaps")
creditorSwap, clean1 := newTestSwap(t, beneficiaryKey)
debitorSwap, clean2 := newTestSwap(t, ownerKey)
debitorSwap, clean2 := newTestSwap(t, issuerKey)
defer clean1()
defer clean2()

Expand All @@ -133,7 +133,7 @@ func TestEmitCheque(t *testing.T) {
// create the debitor peer
dPtpPeer := p2p.NewPeer(enode.ID{}, "debitor", []p2p.Cap{})
dProtoPeer := protocols.NewPeer(dPtpPeer, nil, Spec)
debitor := NewPeer(dProtoPeer, creditorSwap, debitorSwap.owner.address, debitorSwap.owner.Contract)
debitor := NewPeer(dProtoPeer, creditorSwap, debitorSwap.issuer.address, debitorSwap.issuer.Contract)

// set balance artificially
creditorSwap.balances[debitor.ID()] = 42
Expand All @@ -146,13 +146,13 @@ func TestEmitCheque(t *testing.T) {
log.Debug("create a cheque")
cheque := &Cheque{
ChequeParams: ChequeParams{
Contract: debitorSwap.owner.Contract,
Beneficiary: creditorSwap.owner.address,
Contract: debitorSwap.issuer.Contract,
Beneficiary: creditorSwap.issuer.address,
CumulativePayout: 42,
},
Honey: 42,
}
cheque.Signature, err = cheque.Sign(debitorSwap.owner.privateKey)
cheque.Signature, err = cheque.Sign(debitorSwap.issuer.privateKey)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -199,7 +199,7 @@ func TestEmitCheque(t *testing.T) {
// It is the debitor who triggers cheques
func TestTriggerPaymentThreshold(t *testing.T) {
log.Debug("create test swap")
debitorSwap, clean := newTestSwap(t, ownerKey)
debitorSwap, clean := newTestSwap(t, issuerKey)
defer clean()

// setup the wait for mined transaction function for testing
Expand Down
46 changes: 23 additions & 23 deletions swap/swap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,18 +57,18 @@ type Swap struct {
peers map[enode.ID]*Peer // map of all swap Peers
peersLock sync.RWMutex // lock for peers map
backend contract.Backend // the backend (blockchain) used
owner *Owner // contract access
issuer *Issuer // Issuer contract access
params *Params // economic and operational parameters
contract swap.Contract // reference to the smart contract
oracle PriceOracle // the oracle providing the ether price for honey
paymentThreshold int64 // balance difference required for sending cheque
disconnectThreshold int64 // balance difference required for dropping peer
}

// Owner encapsulates information related to accessing the contract
type Owner struct {
// Issuer encapsulates information related to accessing the contract
type Issuer struct {
Contract common.Address // address of swap contract
address common.Address // owner address
address common.Address // issuer address
privateKey *ecdsa.PrivateKey // private key
publicKey *ecdsa.PublicKey // public key
}
Expand All @@ -93,7 +93,7 @@ func New(stateStore state.Store, prvkey *ecdsa.PrivateKey, contract common.Addre
cheques: make(map[enode.ID]*Cheque),
peers: make(map[enode.ID]*Peer),
backend: backend,
owner: createOwner(prvkey, contract),
issuer: createIssuer(prvkey, contract),
params: NewParams(),
paymentThreshold: DefaultPaymentThreshold,
disconnectThreshold: DefaultDisconnectThreshold,
Expand Down Expand Up @@ -126,10 +126,10 @@ func keyToID(key string, prefix string) enode.ID {
return enode.HexID(key[len(prefix):])
}

// createOwner assings keys and addresses
func createOwner(prvkey *ecdsa.PrivateKey, contract common.Address) *Owner {
// createIssuer assings keys and addresses
func createIssuer(prvkey *ecdsa.PrivateKey, contract common.Address) *Issuer {
pubkey := &prvkey.PublicKey
return &Owner{
return &Issuer{
privateKey: prvkey,
publicKey: pubkey,
Contract: contract,
Expand All @@ -139,7 +139,7 @@ func createOwner(prvkey *ecdsa.PrivateKey, contract common.Address) *Owner {

// DeploySuccess is for convenience log output
func (s *Swap) DeploySuccess() string {
return fmt.Sprintf("contract: %s, owner: %s, deposit: %v, signer: %x", s.owner.Contract.Hex(), s.owner.address.Hex(), s.params.InitialDepositAmount, s.owner.publicKey)
return fmt.Sprintf("contract: %s, issuer: %s, deposit: %v, signer: %x", s.issuer.Contract.Hex(), s.issuer.address.Hex(), s.params.InitialDepositAmount, s.issuer.publicKey)
}

// Add is the (sole) accounting function
Expand Down Expand Up @@ -244,7 +244,7 @@ func (s *Swap) handleEmitChequeMsg(ctx context.Context, p *Peer, msg *EmitCheque
return err
}

opts := bind.NewKeyedTransactor(s.owner.privateKey)
opts := bind.NewKeyedTransactor(s.issuer.privateKey)
opts.Context = ctx

otherSwap, err := contract.InstanceAt(cheque.Contract, s.backend)
Expand All @@ -262,7 +262,7 @@ func (s *Swap) handleEmitChequeMsg(ctx context.Context, p *Peer, msg *EmitCheque
// The function cashes the cheque by sending it to the blockchain
func cashCheque(s *Swap, otherSwap contract.Contract, opts *bind.TransactOpts, cheque *Cheque) {
// blocks here, as we are waiting for the transaction to be mined
result, receipt, err := otherSwap.CashChequeBeneficiary(opts, s.backend, s.owner.Contract, big.NewInt(int64(cheque.CumulativePayout)), cheque.Signature)
result, receipt, err := otherSwap.CashChequeBeneficiary(opts, s.backend, s.issuer.Contract, big.NewInt(int64(cheque.CumulativePayout)), cheque.Signature)
if err != nil {
// TODO: do something with the error
// and we actually need to log this error as we are in an async routine; nobody is handling this error for now
Expand All @@ -282,7 +282,7 @@ func cashCheque(s *Swap, otherSwap contract.Contract, opts *bind.TransactOpts, c
// processAndVerifyCheque verifies the cheque and compares it with the last received cheque
// if the cheque is valid it will also be saved as the new last cheque
func (s *Swap) processAndVerifyCheque(cheque *Cheque, p *Peer) (uint64, error) {
if err := cheque.verifyChequeProperties(p, s.owner.address); err != nil {
if err := cheque.verifyChequeProperties(p, s.issuer.address); err != nil {
return 0, err
}

Expand Down Expand Up @@ -405,12 +405,12 @@ func (s *Swap) createCheque(swapPeer *Peer) (*Cheque, error) {
cheque = &Cheque{
ChequeParams: ChequeParams{
CumulativePayout: total + amount,
Contract: s.owner.Contract,
Contract: s.issuer.Contract,
Beneficiary: beneficiary,
},
Honey: honey,
}
cheque.Signature, err = cheque.Sign(s.owner.privateKey)
cheque.Signature, err = cheque.Sign(s.issuer.privateKey)

return cheque, err
}
Expand Down Expand Up @@ -527,8 +527,8 @@ func (s *Swap) verifyContract(ctx context.Context, address common.Address) error
return contract.ValidateCode(ctx, s.backend, address)
}

// 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) {
// getIssuerAtContract retrieve the issuer of the chequebook at address from the blockchain
func (s *Swap) getIssuerAtContract(ctx context.Context, address common.Address) (common.Address, error) {
contr, err := contract.InstanceAt(address, s.backend)
if err != nil {
return common.Address{}, err
Expand All @@ -539,32 +539,32 @@ func (s *Swap) getContractOwner(ctx context.Context, address common.Address) (co

// Deploy deploys the Swap contract
func (s *Swap) Deploy(ctx context.Context, backend swap.Backend, path string) error {
opts := bind.NewKeyedTransactor(s.owner.privateKey)
opts := bind.NewKeyedTransactor(s.issuer.privateKey)
// initial topup value
opts.Value = big.NewInt(int64(s.params.InitialDepositAmount))
opts.Context = ctx

log.Info("deploying new swap", "owner", opts.From.Hex())
address, err := s.deployLoop(opts, backend, s.owner.address, defaultHarddepositTimeoutDuration)
log.Info("deploying new swap", "issuer", opts.From.Hex())
address, err := s.deployLoop(opts, backend, s.issuer.address, defaultHarddepositTimeoutDuration)
if err != nil {
log.Error("unable to deploy swap", "error", err)
return err
}
s.owner.Contract = address
log.Info("swap deployed", "address", address.Hex(), "owner", opts.From.Hex())
s.issuer.Contract = address
log.Info("swap deployed", "address", address.Hex(), "issuer", opts.From.Hex())

return err
}

// deployLoop repeatedly tries to deploy the swap contract .
func (s *Swap) deployLoop(opts *bind.TransactOpts, backend swap.Backend, owner common.Address, defaultHarddepositTimeoutDuration time.Duration) (addr common.Address, err error) {
func (s *Swap) deployLoop(opts *bind.TransactOpts, backend swap.Backend, issuer common.Address, defaultHarddepositTimeoutDuration time.Duration) (addr common.Address, err error) {
var tx *types.Transaction
for try := 0; try < deployRetries; try++ {
if try > 0 {
time.Sleep(deployDelay)
}

if _, s.contract, tx, err = contract.Deploy(opts, backend, owner, defaultHarddepositTimeoutDuration); err != nil {
if _, s.contract, tx, err = contract.Deploy(opts, backend, issuer, defaultHarddepositTimeoutDuration); err != nil {
log.Warn("can't send chequebook deploy tx", "try", try, "error", err)
continue
}
Expand Down
Loading