swap, uint256: unify variable types, pt. 1 #2063

mortelli · 2019-12-24T19:19:56Z

This PR is the first step in implementing issue #1581, which aims at unifying variable types between the SWAP smart contracts and the related code on the go side.

Changes:

Adds the Uint256 type. This struct is merely a wrapper around a big.Int field, but it operates through functions that change its underlying value while controlling over and underflows.
- I expect the Uint64ToUint256 function to be retired once the issue is fully implemented. We should eventually only convert to and from big.Int (which is required by the contract bindings) and Uint256 (or corresponding) types. This should be achieved once all field types are unified.
Changes the CumulativePayout ChequeParams field from the uint64 type to the *Uint256 type.
- as a result, the code and/or signature for the AvailableBalance, encodeForSignature, verifyChequeAgainstLast, (Cheque's) String, getLastSentCumulativePayout, createCheque, handleEmitChequeMsg, cashCheque and processAndVerifyCheque functions has been changed as well.
Update tests to conform with these changes.

…AvailableBalance to use Uint256

…alues, small refactor to Uint256 functions

ralph-pichler · 2020-01-08T12:18:29Z

swap/types.go

+// Sub sets u to minuend - subtrahend and returns u as the difference
+// returns an error when the result falls outside of the unsigned 256-bit integer range
+func (u *Uint256) Sub(minuend, subtrahend *Uint256) (*Uint256, error) {
+	difference := new(big.Int).Sub(minuend.Value, subtrahend.Value)


If we create new big.Ints on every arithmetic operation anyway is there an advantage to having this "set receiver to result" api style? I believe the reason for this design is to avoid allocations happening during normal arithmetic operations but since a new big.Int is allocated here anyway could we not just allocate an entirely new Uint256 and have a more "readable" API like in many other big number implementations (where big nums are immutable):

x = NewUint256(bigIntValue) z = x.Add(y) // x remains unmodified

as you point out, for something like z := x.Add(y) to work (if we want x to remain unmodified) we'd need to call NewUint256 inside of Add, which in my opinion is less compliant with SRP than the current version.

i do see how x, err = x.Add(y) can look better than _, err = x.Add(x,y). although, in this case of doing x += y, x would have its value replaced.

for the record: the "set receiver to result" style was modelled after the math/big Int functions, which was the basis for this design (in the spirit of not re-inventing the wheel).

i don't find it particularly hard to read, but in principle i'm not opposed to changing it.

…-types

holisticode

Before doing any further review, I would like to hear others' opinion about this approach.

While the idea is good, I think there are some implications which I am not sure I agree to or are even correct.

An alternative path could be inspired by https://ethereum.stackexchange.com/a/60699

holisticode · 2020-01-08T17:15:20Z

swap/types.go

+	value big.Int
+}
+
+var minUint256 = big.NewInt(0)


We had this in a different PR, I think these should be constants, not variables

as far as i know, const declarations in general cannot contain function calls, as they must have concrete values at compile time.

we need several function calls to obtain the 2^256-1 number, which cannot even be expressed as an integer literal in go code.

so how could we have these as constants?

By putting the literal (calculated value) for example?

it's too big for a go literal, it won't compile

holisticode · 2020-01-08T17:16:06Z

swap/types.go

+
+// Uint256 represents an unsigned integer of 256 bits
+type Uint256 struct {
+	value big.Int


To be discussed: maybe it is worth considering using a fixed size byte array instead of this

If not a byte array, we should probably return to using a pointer and not the value, otherwise we get unintended consequences

the pointer was removed in the first place to avoid nil values in Uint256 variables. this way, there's always at least an underlying value of 0.

holisticode · 2020-01-08T17:44:03Z

swap/types.go

+// NewUint256 creates a Uint256 struct with a minimum initial underlying value
+func NewUint256() *Uint256 {
+	u := new(Uint256)
+	u.value = *minUint256


This actually sets value to the actual Int instance, check how Int is defined. This is not doing what it looks it is doing....

fmt.Println(u.value)

i'm sorry, i'm not clear on what you mean here.

can you explain what the correct code would be? i'm not seeing the problem with the output of this fmt.Println call.

apologies: if you meant the problem pointed out below in the Set function, it should also be fixed now.

thanks

holisticode · 2020-01-08T19:17:25Z

swap/types.go

+
+// Set assigns the underlying value of the given Uint256 param to u, and returns the modified receiver struct
+// returns an error when the result falls outside of the unsigned 256-bit integer range
+func (u *Uint256) Set(value big.Int) (*Uint256, error) {


Consider this:

v := NewUint256() val := big.NewInt(5) v.Set(*val) // set v to 5 val.SetInt64(9999) // now v AND val have value 9999

Is this intended?

thanks, this was indeed a classic pointer bug.

should be fixed in the latest commit

holisticode · 2020-01-08T19:18:31Z

swap/types.go

+}
+
+// Copy sets the underlying value of u to a copy of the given Uint256 param, and returns the modified receiver struct
+func (u *Uint256) Copy(v *Uint256) *Uint256 {


It is not clear what you want to achieve here; if it is a deep copy, then that is not what is happening, but basically the same as noted in Set

this should be fixed with the latest commit, but to clarify: the intention is to have a sort of constructor function based on an existing *Uint256 var—if these are created correctly (and they were not, as you pointed out) we can safely "clone" them. that's the idea

holisticode · 2020-01-08T19:19:57Z

swap/types.go

+var maxUint256 = new(big.Int).Sub(new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil), big.NewInt(1)) // 2^256 - 1
+
+// NewUint256 creates a Uint256 struct with a minimum initial underlying value
+func NewUint256() *Uint256 {


I would expect to be able to set the new value right in the constructor, like big.NewInt(val)

of course. i will explain my reasoning here: NewUint256 returns only a Uint256 pointer, which is a comfortable condition to have. the same thing happens with the Uint64ToUint256 function.

if we receive a big.Int param, we need to output an err variable as well, since it's possible that the received param falls outside of the allowed range (as it happens in Set).

this would not allow us to do calls like:

newCumulativePayout, err := NewUint256().Add(cumulativePayout, castedPrice)

because the constructor now has 2 variables as output.

however, I am not opposed to adding a new constructor with this behaviour. so instead of:

NewUint256().Set(*tentativeLiquidBalance)

we'd have something like:

NewUint256FromBigInt(*tentativeLiquidBalance)

although my choice for the NewUint256FromBigInt name is pretty bad.

(name suggestions/changes are welcome)

holisticode · 2020-01-08T19:23:43Z

swap/types.go

+	return u.value.Cmp(&v.value)
+}
+
+// Equals returns true if the two Uint256 structs have the same underlying values, false otherwise


Maybe this is not needed, as Cmp does the job

for me, it's more readable to have this:

if !actualAmount.Equals(increase) { ... }

than this:

if actualAmount.Cmp(increase) == 0 { ... }

holisticode · 2020-01-08T19:31:17Z

swap/types.go

+
+// Add sets u to augend + addend and returns u as the sum
+// returns an error when the result falls outside of the unsigned 256-bit integer range
+func (u *Uint256) Add(augend, addend *Uint256) (*Uint256, error) {


If big.Int actually be embedded rather than a field, then we could use directly the big.Int methods for operations

sorry, what do you mean by this? we are already calling big.Int's Add here

nevermind, i see what you mean: you're saying directly through the receiver variable; something like:

type Uint256 struct { big.Int }

no?

but how could we do the over/underflow check then?

Methods woulds still be on the actual type. But never mind, at this stage not needed to explore further.

…oUint256 and Set functions

mortelli · 2020-01-08T21:25:36Z

An alternative path could be inspired by https://ethereum.stackexchange.com/a/60699

we did run into this one a couple of times in the past.

i think it could be part of the solution if we were to implement this through arrays, but we'd still need to implement all the arithmetic functions, and the conversion in the opposite direction, at least.

…-types

…-types # Conflicts: # swap/swap.go # swap/swap_test.go

janos

Would you consider creating a package for Uint256 to reduce the size of the swap package?

janos · 2020-01-14T12:11:00Z

swap/types.go

@@ -49,3 +54,120 @@ type EmitChequeMsg struct {
 type ConfirmChequeMsg struct {
 	Cheque *Cheque
 }
+
+// Uint256 represents an unsigned integer of 256 bits


It looks to me that Uint256 implementation could be in its own file, or even package as it is already exposed.

thanks for the suggestion, this should be implemented through #2076

…ge (#2076) swap, uint256: move Uint256 implementation to new swarm/uint256 package

janos

LGTM. Thanks @mortelli.

uint256/uint256.go

holisticode

A couple of minor things and a more important question:

Should we now not store the balance and use setBalance with Uint256 too?

swap/cheque.go

holisticode · 2020-01-15T14:36:58Z

swap/cheque.go

 	}

 	return actualAmount, nil
 }

 func (cheque *Cheque) String() string {
-	return fmt.Sprintf("Contract: %x Beneficiary: %x CumulativePayout: %d Honey: %d", cheque.Contract, cheque.Beneficiary, cheque.CumulativePayout, cheque.Honey)
+	return fmt.Sprintf("Contract: %x Beneficiary: %x CumulativePayout: %v Honey: %d", cheque.Contract, cheque.Beneficiary, cheque.CumulativePayout, cheque.Honey)


I'd probably print %s, cheque.CumulativePayout.String()

we shouldn't need to call String(), right? Uint256 implements the Stringer interface, ergo it knows how to represent itself as a string without calling the function explicitly, no?

now: do we use %v or %s? as far as i know there is no difference (in practice) as long as the interface is implemented, so i have no preference here.

If it is a String I'd use %s, which is what I actually meant in the first place

swap/peer.go

holisticode · 2020-01-15T14:47:08Z

swap/swap_test.go

@@ -1129,12 +1144,15 @@ func TestPeerProcessAndVerifyChequeInvalid(t *testing.T) {
 	}

 	if peer.getLastReceivedCheque().CumulativePayout != cheque.CumulativePayout {
-		t.Fatalf("last received cheque has wrong cumulative payout, was: %d, expected: %d", peer.lastReceivedCheque.CumulativePayout, cheque.CumulativePayout)
+		t.Fatalf("last received cheque has wrong cumulative payout, was: %v, expected: %v", peer.lastReceivedCheque.CumulativePayout, cheque.CumulativePayout)


I'd print the String

holisticode · 2020-01-15T14:48:36Z

swap/swap_test.go

@@ -1242,8 +1260,8 @@ func TestPeerGetLastSentCumulativePayout(t *testing.T) {
 	_, peer, clean := newTestSwapAndPeer(t, ownerKey)
 	defer clean()

-	if peer.getLastSentCumulativePayout() != 0 {
-		t.Fatalf("last cumulative payout should be 0 in the beginning, was %d", peer.getLastSentCumulativePayout())
+	if !peer.getLastSentCumulativePayout().Equals(uint256.FromUint64(0)) {


Wouldn't it be better to compare just to the New() value instead to run FromUint64?

if you are reading this line by itself, how do you know that:

New() will result in a value which can safely be compared through Equals?

assuming New() results in a safe value, that it is the minimum valid value? (and maybe even: that the minimum valid value is 0)

i will concede that this is a verbose mess considering we just want to say zero. however: i think it's clearer this way.

i am happy to discuss it further though

I am pointing this out only because I have seen elsewhere you have used just that New() for a zero value, for example for the return value for getLastSentCumulativePayout

mortelli · 2020-01-15T19:11:08Z

A couple of minor things and a more important question:

Should we now not store the balance and use setBalance with Uint256 too?

yes! we should. i envisioned this as pt. 2, actually.

i didn't include this change because the branch in its current state is meaty enough for pt. 1—specially considering the discussion that needed to be had given that it was the first part of the solution.

holisticode

Really good job!

mortelli · 2020-01-22T19:26:13Z

A couple of minor things and a more important question:
Should we now not store the balance and use setBalance with Uint256 too?

yes! we should. i envisioned this as pt. 2, actually.

i didn't include this change because the branch in its current state is meaty enough for pt. 1—specially considering the discussion that needed to be had given that it was the first part of the solution.

a note, since i am about to begin pt. 2: balances can actually be negative, so we should use a different type.

i might continue with a different field, but we'll get to that one eventually.

mortelli added 4 commits December 24, 2019 15:40

swap: add skeleton for Uint256

ce6d7e0

swap: iterate Uint256 struct

48e842f

swap: iterate Uint256 struct

d10c32a

swap: iterate Uint256 struct

8e933b0

mortelli added in progress incentives labels Dec 24, 2019

mortelli self-assigned this Dec 24, 2019

mortelli added 23 commits December 24, 2019 16:47

swap: iterate Uint256 struct

3be2ed8

swap: iterate Uint256 struct

44aa724

swap: iterate Uint256 struct

5802fde

swap: iterate Uint256 struct

93722f9

swap: change ChequeParams CumulativePayout field to Uint256, rewrite …

c8a1ee4

…AvailableBalance to use Uint256

swap: add Cmp function for Uint256 type

5ee8279

swap: rewrite verifyChequeAgainstLast function to work with Uint256 v…

068881c

…alues, small refactor to Uint256 functions

swap: iterate Uint256 struct

c4c2a99

swap: iterate Uint256

60b9b99

swap: add Uint64ToUint256 function

388ed44

swap: add Mul function to Uint256 struct

566230f

swap: adapt handleEmitChequeMsg function to work with Uint256

2682e46

swap: iterate Uint256

4dd0330

swap: refactor AvailableBalance

e92d085

swap: iterate Uint256

587b345

swap: iterate Uint256

87a52f1

swap: make tests compile

031b3c4

swap: add MarshalJSON and UnmarshalJSON functions to Uint256

2732ac8

swap: fix nil reference in TestEmitCheque

d38cede

swap: fix bug in encodeForSignature function

933606d

swap: change FromUint64 signature to have no receiver

4d2c0c2

swap: simplify Uint64ToUint256 calls

7b2d982

swap: fix bug in Uint64ToUint256 function

964d564

ralph-pichler reviewed Jan 8, 2020

View reviewed changes

mortelli added 2 commits January 8, 2020 09:24

Merge remote-tracking branch 'origin/master' into swap-unify-variable…

0014fe1

…-types

Merge remote-tracking branch 'origin/master' into swap-unify-variable…

6a469d8

…-types

holisticode reviewed Jan 8, 2020

View reviewed changes

swap: fix unintended shared pointer references in NewUint256, Uint64T…

2025565

…oUint256 and Set functions

mortelli added 2 commits January 9, 2020 15:21

Merge remote-tracking branch 'origin/master' into swap-unify-variable…

d48d980

…-types

swap: small change to TestCopyUint256 func

b45c506

zelig approved these changes Jan 11, 2020

View reviewed changes

mortelli added 2 commits January 13, 2020 10:47

Merge remote-tracking branch 'origin/master' into swap-unify-variable…

810f75f

…-types # Conflicts: # swap/swap.go # swap/swap_test.go

swap: small refactor of Uint256 funcs after merge

5c2ddde

janos reviewed Jan 14, 2020

View reviewed changes

mortelli mentioned this pull request Jan 14, 2020

swap, uint256: move Uint256 implementation to new swarm/uint256 package #2076

Merged

swap, uint256: move Uint256 implementation to new swarm/uint256 packa…

74043d7

…ge (#2076) swap, uint256: move Uint256 implementation to new swarm/uint256 package

mortelli changed the title ~~swap: unify variable types, pt. 1~~ swap, uint256: unify variable types, pt. 1 Jan 15, 2020

mortelli added ready for another review and removed ready for review labels Jan 15, 2020

janos approved these changes Jan 15, 2020

View reviewed changes

holisticode reviewed Jan 15, 2020

View reviewed changes

uint256/uint256.go Show resolved Hide resolved

holisticode reviewed Jan 15, 2020

View reviewed changes

uint256/uint256.go Outdated Show resolved Hide resolved

holisticode reviewed Jan 15, 2020

View reviewed changes

uint256: call Set on Uint256 UnmarshalJSON and DecodeRLP funcs

3a400df

holisticode approved these changes Jan 15, 2020

View reviewed changes

mortelli merged commit 0bc6bef into master Jan 15, 2020

acud added this to the 0.5.5 milestone Jan 21, 2020

mortelli deleted the swap-unify-variable-types branch January 22, 2020 19:26

mortelli mentioned this pull request Jan 28, 2020

swap: unify variable types, pt. 2 #2082

Merged

mortelli mentioned this pull request Feb 25, 2020

swap, int256, boundedint: unify variable types, pt. 3 #2085

Merged

swap, uint256: unify variable types, pt. 1 #2063

swap, uint256: unify variable types, pt. 1 #2063

Conversation

mortelli commented Dec 24, 2019 • edited Loading

Choose a reason for hiding this comment

mortelli Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

holisticode left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortelli Jan 8, 2020 • 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

mortelli Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortelli Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortelli Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortelli Jan 8, 2020 • 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

mortelli Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortelli commented Jan 8, 2020 • edited Loading

janos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janos left a comment

Choose a reason for hiding this comment

holisticode 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 Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortelli commented Jan 15, 2020

holisticode left a comment

Choose a reason for hiding this comment

mortelli commented Jan 22, 2020 • edited Loading

mortelli commented Dec 24, 2019 •

edited

Loading

mortelli Jan 8, 2020 •

edited

Loading

holisticode left a comment •

edited

Loading

mortelli Jan 8, 2020 •

edited

Loading

mortelli Jan 8, 2020 •

edited

Loading

mortelli Jan 8, 2020 •

edited

Loading

mortelli Jan 8, 2020 •

edited

Loading

mortelli Jan 8, 2020 •

edited

Loading

mortelli Jan 8, 2020 •

edited

Loading

mortelli commented Jan 8, 2020 •

edited

Loading

mortelli Jan 15, 2020 •

edited

Loading

mortelli commented Jan 22, 2020 •

edited

Loading