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

Enforce fair outcome in direct and virtual fund objectives #1610

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions internal/testdata/states.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ var testVirtualState = state.State{
ChallengeDuration: 60,
AppData: []byte{},
Outcome: Outcomes.CreateLongOutcome(
SimpleItem{testactors.Alice.Destination(), 6},
SimpleItem{testactors.Bob.Destination(), 4},
SimpleItem{testactors.Alice.Destination(), 10},
SimpleItem{testactors.Bob.Destination(), 0},
),
TurnNum: 0,
IsFinal: false,
Expand Down
8 changes: 6 additions & 2 deletions packages/nitro-rpc-client/src/rpc-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ import {
ReceiveVoucherResult,
} from "./types";
import { Transport } from "./transport";
import { createOutcome, generateRequest } from "./utils";
import {
createOutcome,
createPaymentChannelOutcome,
generateRequest,
} from "./utils";
import { HttpTransport } from "./transport/http";
import { getAndValidateResult } from "./serde";

Expand Down Expand Up @@ -120,7 +124,7 @@ export class NitroRpcClient {
CounterParty: counterParty,
Intermediaries: intermediaries,
ChallengeDuration: 0,
Outcome: createOutcome(
Outcome: createPaymentChannelOutcome(
asset,
await this.GetAddress(),
counterParty,
Expand Down
40 changes: 40 additions & 0 deletions packages/nitro-rpc-client/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,46 @@ export function createOutcome(
},
];
}
/**
* createPaymentChannelOutcome creates a basic outcome for a payment channel
*
* @param asset - The asset to fund the channel with
* @param alpha - The address of the first participant
* @param beta - The address of the second participant
* @param amount - The amount to allocate to the payer
* @returns An outcome for a virtually funded channel
*/
export function createPaymentChannelOutcome(
asset: string,
alpha: string,
beta: string,
amount: number
): Outcome {
return [
{
Asset: asset,
AssetMetadata: {
AssetType: 0,
Metadata: null,
},

Allocations: [
{
Destination: convertAddressToBytes32(alpha),
Amount: amount,
AllocationType: 0,
Metadata: null,
},
{
Destination: convertAddressToBytes32(beta),
Amount: 0,
AllocationType: 0,
Metadata: null,
},
],
},
];
}

/**
* Left pads a 20 byte address hex string with zeros until it is a 32 byte hex string
Expand Down
17 changes: 17 additions & 0 deletions protocols/directfund/directfund.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,19 @@ func ChannelsExistWithCounterparty(counterparty types.Address, getChannels GetCh
return ok, nil
}

// hasEqualOutcome returns true if the outcome allocates an equal amount to each participant
func hasEqualOutcome(s state.State, me types.Address) bool {
Comment on lines +113 to +114
Copy link
Contributor

Choose a reason for hiding this comment

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

hasUniformOutcome is better, I think?

for _, e := range s.Outcome {
forMe := e.TotalAllocatedFor(types.AddressToDestination(me))
for _, a := range e.Allocations {
if a.Amount.Cmp(forMe) != 0 {
return false
}
}
}
return true
}

// ConstructFromPayload initiates a Objective with data calculated from
// the supplied initialState and client address
func ConstructFromPayload(
Expand All @@ -124,6 +137,7 @@ func ConstructFromPayload(
return Objective{}, fmt.Errorf("could not get signed state payload: %w", err)
}
initialState := initialSignedState.State()

err = initialState.FixedPart().Validate()
if err != nil {
return Objective{}, err
Expand All @@ -135,6 +149,9 @@ func ConstructFromPayload(
return Objective{}, errors.New("attempted to initiate new direct-funding objective with IsFinal == true")
}

if !hasEqualOutcome(initialState, myAddress) {
return Objective{}, errors.New("attempted to initiate new direct-funding objective with non-equal outcome")
}
Comment on lines +152 to +154
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow on changes here if you like "uniform outcome".

init := Objective{}

if preApprove {
Expand Down
13 changes: 9 additions & 4 deletions protocols/directfund/directfund_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func TestNew(t *testing.T) {
request := NewObjectiveRequest(
testState.Participants[1],
testState.ChallengeDuration,
testState.Outcome,
testState.Outcome.Clone(),
0,
testState.AppDefinition,
)
Expand All @@ -69,7 +69,7 @@ func TestNew(t *testing.T) {
}

getByParticipantHasChannel := func(id types.Address) ([]*channel.Channel, error) {
c, _ := channel.New(testState, 0)
c, _ := channel.New(testState.Clone(), 0)
return []*channel.Channel{c}, nil
}

Expand All @@ -83,10 +83,15 @@ func TestNew(t *testing.T) {
if _, err := NewObjective(request, false, testState.Participants[0], big.NewInt(TEST_CHAIN_ID), getByParticipant, getByConsensusHasChannel); err == nil {
t.Errorf("Expected an error when constructing with an objective when an existing channel consensus channel exists")
}

request.Outcome[0].Allocations[0].Amount = big.NewInt(10)
if _, err := NewObjective(request, false, testState.Participants[0], big.NewInt(TEST_CHAIN_ID), getByParticipant, getByConsensus); err == nil {
t.Errorf("Expected an error when constructing a direct fund objective with an unfair outcome")
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
t.Errorf("Expected an error when constructing a direct fund objective with an unfair outcome")
t.Errorf("Expected an error when constructing a direct fund objective with an non-uniform outcome")

}
}

func TestConstructFromPayload(t *testing.T) {
ss := state.NewSignedState(testState)
ss := state.NewSignedState(testState.Clone())
id := protocols.ObjectiveId(ObjectivePrefix + testState.ChannelId().String())
op, err := protocols.CreateObjectivePayload(id, SignedStatePayload, ss)
testhelpers.Ok(t, err)
Expand All @@ -113,7 +118,7 @@ func TestConstructFromPayload(t *testing.T) {

func TestUpdate(t *testing.T) {
id := protocols.ObjectiveId(ObjectivePrefix + testState.ChannelId().String())
op, err := protocols.CreateObjectivePayload(id, SignedStatePayload, state.NewSignedState(testState))
op, err := protocols.CreateObjectivePayload(id, SignedStatePayload, state.NewSignedState(testState.Clone()))
testhelpers.Ok(t, err)
// Construct various variables for use in TestUpdate
s, _ := ConstructFromPayload(false, op, testState.Participants[0])
Expand Down
8 changes: 4 additions & 4 deletions protocols/virtualfund/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,16 @@ import (
)

// prepareConsensusChannel prepares a consensus channel with a consensus outcome
// - allocating 6 to left participant
// - allocating 4 to right participant
// - allocating 5 to left participant
// - allocating 5 to right participant
// - including the given guarantees
func prepareConsensusChannel(role uint, leader, follower, leftActor testactors.Actor, guarantees ...consensus_channel.Guarantee) *consensus_channel.ConsensusChannel {
return prepareConsensusChannelHelper(role, leader, follower, leftActor, 6, 4, 1, guarantees...)
return prepareConsensusChannelHelper(role, leader, follower, leftActor, 5, 5, 1, guarantees...)
}

// consensusStateSignatures prepares a consensus channel with a consensus outcome and returns the signatures on the consensus state
func consensusStateSignatures(leader, follower testactors.Actor, guarantees ...consensus_channel.Guarantee) [2]state.Signature {
return prepareConsensusChannelHelper(0, leader, follower, leader, 0, 0, 2, guarantees...).Signatures()
return prepareConsensusChannelHelper(0, leader, follower, leader, 0, 5, 2, guarantees...).Signatures()
}

func prepareConsensusChannelHelper(role uint, leader, follower, leftActor testactors.Actor, leftBalance, rightBalance, turnNum int, guarantees ...consensus_channel.Guarantee) *consensus_channel.ConsensusChannel {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope nit: I wonder if there's a more useful way to name this function. "Helper" doesn't really tell us anything...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out of scope but I think the virtual fund tests could use some refactoring in general. Helpers within helpers relying of specific test data made updating this code slightly tricky.

Expand Down
20 changes: 20 additions & 0 deletions protocols/virtualfund/virtualfund.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/statechannels/go-nitro/channel/consensus_channel"
"github.com/statechannels/go-nitro/channel/state"
"github.com/statechannels/go-nitro/channel/state/outcome"
"github.com/statechannels/go-nitro/payments"
"github.com/statechannels/go-nitro/protocols"
"github.com/statechannels/go-nitro/types"
)
Expand Down Expand Up @@ -132,6 +133,21 @@ type Objective struct {
b0 types.Funds // Initial balance for Bob
}

// onlyPayerHasFunds checks that the outcome only has funds allocated to the payer
func onlyPayerHasFunds(s state.State) bool {
for _, e := range s.Outcome {
total := e.TotalAllocated()
for i, a := range e.Allocations {
if i == payments.PAYER_INDEX && a.Amount.Cmp(total) != 0 {
return false
} else if i != payments.PAYER_INDEX && a.Amount.Cmp(big.NewInt(0)) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit but you don't need the else here, and I believe it is encouraged to omit it (saves some indentation).

return false
}
}
}
return true
}

// NewObjective creates a new virtual funding objective from a given request.
func NewObjective(request ObjectiveRequest, preApprove bool, myAddress types.Address, chainId *big.Int, getTwoPartyConsensusLedger GetTwoPartyConsensusLedgerFunction) (Objective, error) {
var rightCC *consensus_channel.ConsensusChannel
Expand Down Expand Up @@ -185,6 +201,10 @@ func constructFromState(
init.Status = protocols.Unapproved
}

if !onlyPayerHasFunds(initialStateOfV) {
return Objective{}, errors.New("initial state of V has funds allocated to non-payer")
}

// Infer MyRole
found := false
for i, addr := range initialStateOfV.Participants {
Expand Down
40 changes: 31 additions & 9 deletions protocols/virtualfund/virtualfund_single_hop_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ func newTestData() testData {
Allocations: outcome.Allocations{
outcome.Allocation{
Destination: alice.Destination(),
Amount: big.NewInt(6),
Amount: big.NewInt(5),
},
outcome.Allocation{
Destination: bob.Destination(),
Amount: big.NewInt(4),
Amount: big.NewInt(0),
},
},
}},
Expand Down Expand Up @@ -149,6 +149,14 @@ func TestNew(t *testing.T) {
msg := fmt.Sprintf("Testing new as %v", a.Name)
t.Run(msg, testNew(a))
}
td := newTestData()
vPreFund := td.vPreFund
ledgers := td.leaderLedgers
vPreFund.Outcome[0].Allocations[1].Amount = big.NewInt(10)
_, err := constructFromState(false, vPreFund, ta.Alice.Address(), ledgers[ta.Alice.Destination()].left, ledgers[ta.Alice.Destination()].right)
if err == nil {
t.Fatal("Expected error when creating objective with non-zero payee outcome")
}
}

func testCloneAs(my ta.Actor) Tester {
Expand Down Expand Up @@ -223,6 +231,7 @@ func TestCrankAsAlice(t *testing.T) {
ledgers = td.leaderLedgers
s, _ = constructFromState(false, vPreFund, my.Address(), ledgers[my.Destination()].left, ledgers[my.Destination()].right)
)

// Assert that cranking an unapproved objective returns an error
_, _, _, err := s.Crank(&my.PrivateKey)
Assert(t, err != nil, `Expected error when cranking unapproved objective, but got nil`)
Expand Down Expand Up @@ -264,8 +273,12 @@ func TestCrankAsAlice(t *testing.T) {
oObj, effects, waitingFor, err = o.Crank(&my.PrivateKey)
o = oObj.(*Objective)

p := consensus_channel.NewAddProposal(o.ToMyRight.Channel.Id, o.ToMyRight.getExpectedGuarantee(), big.NewInt(6))
sp := consensus_channel.SignedProposal{Proposal: p, Signature: consensusStateSignatures(alice, p1, o.ToMyRight.getExpectedGuarantee())[0], TurnNum: 2}
p := consensus_channel.NewAddProposal(o.ToMyRight.Channel.Id, o.ToMyRight.getExpectedGuarantee(), big.NewInt(5))
sp := consensus_channel.SignedProposal{
Proposal: p,
Signature: prepareConsensusChannelHelper(0, alice, p1, alice, 0, 5, 2, o.ToMyRight.getExpectedGuarantee()).Signatures()[0],
TurnNum: 2,
}
Ok(t, err)
assertOneProposalSent(t, effects, sp, p1)
Equals(t, waitingFor, WaitingForCompleteFunding)
Expand Down Expand Up @@ -361,8 +374,13 @@ func TestCrankAsBob(t *testing.T) {
Equals(t, waitingFor, WaitingForCompleteFunding)

// If Bob had received a signed counterproposal, he should proceed to postFundSetup
p := consensus_channel.NewAddProposal(o.ToMyLeft.Channel.Id, o.ToMyLeft.getExpectedGuarantee(), big.NewInt(6))
sp := consensus_channel.SignedProposal{Proposal: p, Signature: consensusStateSignatures(p1, bob, o.ToMyLeft.getExpectedGuarantee())[0], TurnNum: 2}

p := consensus_channel.NewAddProposal(o.ToMyLeft.Channel.Id, o.ToMyLeft.getExpectedGuarantee(), big.NewInt(5))
sp := consensus_channel.SignedProposal{
Proposal: p,
Signature: prepareConsensusChannelHelper(0, p1, bob, p1, 0, 5, 2, o.ToMyLeft.getExpectedGuarantee()).Signatures()[0],
TurnNum: 2,
}

oObj, err = o.ReceiveProposal(sp)
o = oObj.(*Objective)
Expand Down Expand Up @@ -431,8 +449,12 @@ func TestCrankAsP1(t *testing.T) {
oObj, effects, waitingFor, err = o.Crank(&my.PrivateKey)
o = oObj.(*Objective)

p := consensus_channel.NewAddProposal(o.ToMyLeft.Channel.Id, o.ToMyLeft.getExpectedGuarantee(), big.NewInt(6))
sp := consensus_channel.SignedProposal{Proposal: p, Signature: consensusStateSignatures(p1, alice, o.ToMyLeft.getExpectedGuarantee())[0], TurnNum: 2}
p := consensus_channel.NewAddProposal(o.ToMyLeft.Channel.Id, o.ToMyLeft.getExpectedGuarantee(), big.NewInt(5))
sp := consensus_channel.SignedProposal{
Proposal: p,
Signature: prepareConsensusChannelHelper(0, p1, alice, p1, 5, 0, 2, o.ToMyLeft.getExpectedGuarantee()).Signatures()[0],
TurnNum: 2,
}
Ok(t, err)
assertOneProposalSent(t, effects, sp, alice)
Equals(t, waitingFor, WaitingForCompleteFunding)
Expand All @@ -446,7 +468,7 @@ func TestCrankAsP1(t *testing.T) {
Equals(t, waitingFor, WaitingForCompleteFunding)

// If P1 had received a signed counterproposal, she should proceed to postFundSetup
p = consensus_channel.NewAddProposal(o.ToMyLeft.Channel.Id, o.ToMyLeft.getExpectedGuarantee(), big.NewInt(6))
p = consensus_channel.NewAddProposal(o.ToMyLeft.Channel.Id, o.ToMyLeft.getExpectedGuarantee(), big.NewInt(5))
sp = consensus_channel.SignedProposal{Proposal: p, Signature: consensusStateSignatures(p1, alice, o.ToMyLeft.getExpectedGuarantee())[1], TurnNum: 2}

oObj, err = o.ReceiveProposal(sp)
Expand Down
4 changes: 2 additions & 2 deletions protocols/virtualfund/virtualfund_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ func TestMarshalJSON(t *testing.T) {
Allocations: outcome.Allocations{
outcome.Allocation{
Destination: alice.Destination(),
Amount: big.NewInt(5),
Amount: big.NewInt(10),
},
outcome.Allocation{
Destination: bob.Destination(),
Amount: big.NewInt(5),
Amount: big.NewInt(0),
},
},
}},
Expand Down