Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define an abstract type for wallet.Sig #293

35 changes: 32 additions & 3 deletions backend/ethereum/channel/adjudicator.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ import (
"perun.network/go-perun/backend/ethereum/bindings"
"perun.network/go-perun/backend/ethereum/bindings/adjudicator"
cherrors "perun.network/go-perun/backend/ethereum/channel/errors"
ethwallet "perun.network/go-perun/backend/ethereum/wallet"
"perun.network/go-perun/channel"
"perun.network/go-perun/client"
"perun.network/go-perun/log"
"perun.network/go-perun/wallet"
psync "polycry.pt/poly-go/sync"
)

Expand Down Expand Up @@ -81,7 +83,11 @@ func (a *Adjudicator) Progress(ctx context.Context, req channel.ProgressReq) err
state adjudicator.ChannelState,
_ [][]byte,
) (*types.Transaction, error) {
return a.contract.Progress(opts, params, state, ethNewState, ethActorIndex, req.Sig)
sigBytes, err := req.Sig.MarshalBinary()
if err != nil {
return nil, errors.WithMessage(err, "converting to byte array")
}
return a.contract.Progress(opts, params, state, ethNewState, ethActorIndex, sigBytes)
Comment on lines +86 to +90
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit like you are mixing concerns here. MarshalBinary should typically be used by a marshaler that doesn't know much about the concrete type and just wants some byte representation. However, here we know the type and the contract expects a very specific byte representation.

Suggested change
sigBytes, err := req.Sig.MarshalBinary()
if err != nil {
return nil, errors.WithMessage(err, "converting to byte array")
}
return a.contract.Progress(opts, params, state, ethNewState, ethActorIndex, sigBytes)
sig, ok := req.Sig.(*ethwallet.Sig)
if !ok {
return nil, errors.Errorf("invalid signature type: %T", req.Sig)
}
return a.contract.Progress(opts, params, state, ethNewState, ethActorIndex, *sig)

}
return a.call(ctx, req.AdjudicatorReq, progress, Progress)
}
Expand All @@ -105,12 +111,35 @@ func toEthSignedStates(subChannels []channel.SignedState) (ethSubChannels []adju
ethSubChannels[i] = adjudicator.AdjudicatorSignedState{
Params: ToEthParams(x.Params),
State: ToEthState(x.State),
Sigs: x.Sigs,
Sigs: sigsToByteArrays(x.Sigs),
}
}
return
}

func sigsToByteArrays(signs []wallet.Sig) [][]byte {
byteArrays := make([][]byte, len(signs))
var err error
for i := range signs {
if signs[i] != nil {
byteArrays[i], err = signs[i].MarshalBinary()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as above (regarding concerns of MarshalBinary vs byte representation for a contract call) applies here.

if err != nil {
panic("Marshalling to Binary should not error: " + err.Error())
}
}
}
return byteArrays
}

func byteArraysToSigs(byteArrays [][]byte) []wallet.Sig {
sigs := make([]wallet.Sig, len(byteArrays))
for i := range byteArrays {
sig := (ethwallet.Sig)(byteArrays[i])
sigs[i] = &sig
}
return sigs
}

func (a *Adjudicator) callConclude(ctx context.Context, req channel.AdjudicatorReq, subStates channel.StateMap) error {
ethSubStates := toEthSubStates(req.Tx.State, subStates)

Expand Down Expand Up @@ -153,7 +182,7 @@ func (a *Adjudicator) call(ctx context.Context, req channel.AdjudicatorReq, fn a
if err != nil {
return nil, errors.WithMessage(err, "creating transactor")
}
tx, err := fn(trans, ethParams, ethState, req.Tx.Sigs)
tx, err := fn(trans, ethParams, ethState, sigsToByteArrays(req.Tx.Sigs))
if err != nil {
err = cherrors.CheckIsChainNotReachableError(err)
return nil, errors.WithMessage(err, "calling adjudicator function")
Expand Down
3 changes: 2 additions & 1 deletion backend/ethereum/channel/adjudicator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
ethwallettest "perun.network/go-perun/backend/ethereum/wallet/test"
"perun.network/go-perun/channel"
channeltest "perun.network/go-perun/channel/test"
"perun.network/go-perun/wallet"
pkgtest "polycry.pt/poly-go/test"
)

Expand All @@ -43,7 +44,7 @@ func testSignState(t *testing.T, accounts []*keystore.Account, state *channel.St
}

func signState(accounts []*keystore.Account, state *channel.State) (channel.Transaction, error) {
sigs := make([][]byte, len(accounts))
sigs := make([]wallet.Sig, len(accounts))
for i := range accounts {
sig, err := channel.Sign(accounts[i], state)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion backend/ethereum/channel/subscription.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func (a *Adjudicator) convertEvent(ctx context.Context, e *adjudicator.Adjudicat
return &channel.RegisteredEvent{
AdjudicatorEventBase: *base,
State: &state,
Sigs: ch.Sigs,
Sigs: byteArraysToSigs(ch.Sigs),
}, nil

case phaseForceExec:
Expand Down
3 changes: 1 addition & 2 deletions backend/ethereum/channel/test/transactor.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"github.com/stretchr/testify/require"

"perun.network/go-perun/backend/ethereum/channel"
"perun.network/go-perun/backend/ethereum/wallet"
)

// TransactorSetup holds the setup for running generic tests on a transactor implementation.
Expand Down Expand Up @@ -90,7 +89,7 @@ func sigFromRSV(t *testing.T, r, s, _v *big.Int, chainID int64) []byte {
sigVAdd = 35
sigVSubtract = 27
)
sig := make([]byte, wallet.SigLen)
sig := make([]byte, sigLen)
copy(sig[elemLen-len(r.Bytes()):elemLen], r.Bytes())
copy(sig[elemLen*2-len(s.Bytes()):elemLen*2], s.Bytes())
v := byte(_v.Uint64()) // Needed for chain ids > 110.
Expand Down
7 changes: 6 additions & 1 deletion backend/ethereum/channel/withdraw.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,12 @@ func (a *Adjudicator) newWithdrawalAuth(request channel.AdjudicatorReq, asset as
}

sig, err := request.Acc.SignData(enc)
return auth, sig, errors.WithMessage(err, "sign data")
if err != nil {
return assetholder.AssetHolderWithdrawalAuth{}, nil, errors.WithMessage(err, "sign data")
}
sigBytes, err := sig.MarshalBinary()
Copy link
Contributor

Choose a reason for hiding this comment

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

same comments as above regarding byte representation for contract call applies here.


return auth, sigBytes, errors.WithMessage(err, "marshalling signature into bytes")
}

func encodeAssetHolderWithdrawalAuth(auth assetholder.AssetHolderWithdrawalAuth) ([]byte, error) {
Expand Down
48 changes: 20 additions & 28 deletions backend/ethereum/wallet/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,27 +15,15 @@
package wallet

import (
"io"

"github.com/ethereum/go-ethereum/crypto"
"github.com/pkg/errors"

"perun.network/go-perun/wallet"
"perun.network/go-perun/wire/perunio"
)

// Backend implements the utility interface defined in the wallet package.
type Backend struct{}

// SigLen length of a signature in byte.
// ref https://godoc.org/github.com/ethereum/go-ethereum/crypto/secp256k1#Sign
// ref https://github.com/ethereum/go-ethereum/blob/54b271a86dd748f3b0bcebeaf678dc34e0d6177a/crypto/signature_cgo.go#L66
const SigLen = 65

// sigVSubtract value that is subtracted from the last byte of a signature if
// the last bytes exceeds it.
const sigVSubtract = 27

// compile-time check that the ethereum backend implements the perun backend.
var _ wallet.Backend = (*Backend)(nil)

Expand All @@ -46,31 +34,35 @@ func (b *Backend) NewAddress() wallet.Address {
return &addr
}

// DecodeSig reads a []byte with length of an ethereum signature.
func (*Backend) DecodeSig(r io.Reader) (wallet.Sig, error) {
return DecodeSig(r)
// NewSig returns a variable of type Sig, which can be used for unmarshalling a
// signature from its binary representation.
func (*Backend) NewSig() wallet.Sig {
sig := Sig(make([]byte, sigLen))
return &sig
}

// VerifySignature verifies a signature.
// VerifySignature verifies if the signature on the given message was made by
// this account.
func (*Backend) VerifySignature(msg []byte, sig wallet.Sig, a wallet.Address) (bool, error) {
return VerifySignature(msg, sig, a)
}

// DecodeSig reads a []byte with length of an ethereum signature.
func DecodeSig(r io.Reader) (wallet.Sig, error) {
buf := make(wallet.Sig, SigLen)
return buf, perunio.Decode(r, &buf)
}

// VerifySignature verifies if a signature was made by this account.
// VerifySignature verifies if the signature on the given message was made by
// this account.
func VerifySignature(msg []byte, sig wallet.Sig, a wallet.Address) (bool, error) {
ethSig, ok := sig.(*Sig)
if !ok {
return false, errors.New("signature was not of the expected type")
}

hash := PrefixedHash(msg)
sigCopy := make([]byte, SigLen)
copy(sigCopy, sig)
if len(sigCopy) == SigLen && (sigCopy[SigLen-1] >= sigVSubtract) {
sigCopy[SigLen-1] -= sigVSubtract
ethSigCopy := make([]byte, sigLen)
copy(ethSigCopy, *ethSig)
if ethSigCopy[sigLen-1] >= sigVSubtract {
ethSigCopy[sigLen-1] -= sigVSubtract
}
Comment on lines +59 to 63
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a comment that explains what is happening here.

pk, err := crypto.SigToPub(hash, sigCopy)

pk, err := crypto.SigToPub(hash, ethSigCopy)
if err != nil {
return false, errors.WithStack(err)
}
Expand Down
5 changes: 3 additions & 2 deletions backend/ethereum/wallet/hd/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,15 @@ func (a *Account) Address() wallet.Address {
}

// SignData is used to sign data with this account.
func (a *Account) SignData(data []byte) ([]byte, error) {
func (a *Account) SignData(data []byte) (wallet.Sig, error) {
hash := crypto.Keccak256(data)
sig, err := a.wallet.SignText(a.Account, hash)
if err != nil {
return nil, errors.Wrap(err, "SignText")
}
sig[64] += 27
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a comment that explains what is happening here.

Should use a const instead of 27. Why is the linter not catching this?

return sig, nil
ethSig := (ethwallet.Sig)(sig)
return &ethSig, nil
}

// NewAccountFromEth creates a new perun account from a given ethereum account.
Expand Down
5 changes: 3 additions & 2 deletions backend/ethereum/wallet/keystore/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,15 @@ func (a *Account) Address() wallet.Address {
}

// SignData is used to sign data with this account.
func (a *Account) SignData(data []byte) ([]byte, error) {
func (a *Account) SignData(data []byte) (wallet.Sig, error) {
hash := ethwallet.PrefixedHash(data)
sig, err := a.wallet.Ks.SignHash(a.Account, hash)
if err != nil {
return nil, errors.Wrap(err, "SignHash")
}
sig[64] += 27
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a comment that explains what is happening here.

Should use a const instead of 27. Why is the linter not catching this?

(Could move this part to a central location that is used by the other Ethereum wallet implementations as well.)

return sig, nil
ethSig := (ethwallet.Sig)(sig)
return &ethSig, nil
}

// NewAccountFromEth creates a new perun account from a given ethereum account.
Expand Down
4 changes: 2 additions & 2 deletions backend/ethereum/wallet/keystore/wallet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ func TestSignatures(t *testing.T) {
acc := ethwallettest.NewTmpWallet().NewAccount()
sign, err := acc.SignData(dataToSign)
assert.NoError(t, err, "Sign with new account should succeed")
assert.Equal(t, len(sign), ethwallet.SigLen, "Ethereum signature has wrong length")
valid, err := new(ethwallet.Backend).VerifySignature(dataToSign, sign, acc.Address())
assert.True(t, valid, "Verification should succeed")
assert.NoError(t, err, "Verification should succeed")
Expand Down Expand Up @@ -113,7 +112,8 @@ func TestCurve_SigningAndVerifying(t *testing.T) {
sig, err := hex.DecodeString("538da6430f7915832de165f89c69239020461b80861559a00d4f5a2a7705765219eb3969eb7095f8addb6bf9c9f96f6adf44cfd4a8136516f88b337a428bf1bb1b")
require.NoError(t, err, "decode sig should not error")
addr := ethwallet.Address(common.HexToAddress("f17f52151EbEF6C7334FAD080c5704D77216b732"))
b, err := ethwallet.VerifySignature(msg, sig, &addr)
ethSig := ethwallet.Sig(sig)
b, err := ethwallet.VerifySignature(msg, &ethSig, &addr)
assert.NoError(t, err, "VerifySignature should not error")
assert.True(t, b, "VerifySignature")
}
57 changes: 57 additions & 0 deletions backend/ethereum/wallet/sig.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Copyright 2019 - See NOTICE file for copyright holders.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package wallet

import (
"fmt"

"perun.network/go-perun/wallet"
)

const (
// sigLen length of a signature in byte.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// sigLen length of a signature in byte.
// sigLen is the length of a signature in byte.

// ref https://godoc.org/github.com/ethereum/go-ethereum/crypto/secp256k1#Sign
// ref https://github.com/ethereum/go-ethereum/blob/54b271a86dd748f3b0bcebeaf678dc34e0d6177a/crypto/signature_cgo.go#L66
sigLen = 65

// sigVSubtract value that is subtracted from the last byte of a signature if
// the last bytes exceeds it.
sigVSubtract = 27
)

// Sig represents a signature generated using an ethereum account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Sig represents a signature generated using an ethereum account.
// Sig represents a signature generated using an Ethereum account.

type Sig []byte

// MarshalBinary marshals the signature into its binary representation. Error
// will always be nil, it is for implementing BinaryMarshaler.
func (s Sig) MarshalBinary() ([]byte, error) {
return s[:], nil
}

// UnmarshalBinary unmarshals the signature from its binary representation.
func (s *Sig) UnmarshalBinary(data []byte) error {
if len(data) != sigLen {
return fmt.Errorf("unexpected signature length %d, want %d", len(data), sigLen) //nolint: goerr113
}
copy(*s, data)
return nil
}

// Clone returns a deep copy of the signature.
func (s Sig) Clone() wallet.Sig {
clone := Sig(make([]byte, sigLen))
copy(clone, s)
return &clone
}
5 changes: 3 additions & 2 deletions backend/ethereum/wallet/simple/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,15 @@ func (a *Account) Address() wallet.Address {
}

// SignData is used to sign data with this account.
func (a *Account) SignData(data []byte) ([]byte, error) {
func (a *Account) SignData(data []byte) (wallet.Sig, error) {
hash := ethwallet.PrefixedHash(data)
sig, err := a.SignHash(hash)
if err != nil {
return nil, errors.Wrap(err, "SignHash")
}
sig[64] += 27
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a comment that explains what is happening here.

Should use a const instead of 27. Why is the linter not catching this?

(Could move this part to a central location that is used by the other Ethereum wallet implementations as well.)

return sig, nil
ethSig := (ethwallet.Sig)(sig)
return &ethSig, nil
}

// SignHash is used to sign an already prefixed hash with this account.
Expand Down
1 change: 0 additions & 1 deletion backend/ethereum/wallet/simple/wallet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ func TestSignatures(t *testing.T) {
sig, err := acc.SignData(dataToSign)
assert.NoError(t, err, "Sign with new account should succeed")
assert.NotNil(t, sig)
assert.Equal(t, len(sig), ethwallet.SigLen, "Ethereum signature has wrong length")
valid, err := new(ethwallet.Backend).VerifySignature(dataToSign, sig, acc.Address())
assert.True(t, valid, "Verification should succeed")
assert.NoError(t, err, "Verification should succeed")
Expand Down
4 changes: 2 additions & 2 deletions backend/sim/channel/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (*backend) CalcID(p *channel.Params) (id channel.ID) {
}

// Sign signs `state`.
func (b *backend) Sign(addr wallet.Account, state *channel.State) ([]byte, error) {
func (b *backend) Sign(addr wallet.Account, state *channel.State) (wallet.Sig, error) {
log.WithFields(log.Fields{"channel": state.ID, "version": state.Version}).Tracef("Signing state")

buff := new(bytes.Buffer)
Expand All @@ -65,7 +65,7 @@ func (b *backend) Sign(addr wallet.Account, state *channel.State) ([]byte, error
}

// Verify verifies the signature for `state`.
func (b *backend) Verify(addr wallet.Address, state *channel.State, sig []byte) (bool, error) {
func (b *backend) Verify(addr wallet.Address, state *channel.State, sig wallet.Sig) (bool, error) {
buff := new(bytes.Buffer)
if err := state.Encode(buff); err != nil {
return false, errors.WithMessage(err, "pack state")
Expand Down
Loading