accounts/abi: reorganizing package with small fixes by VoR0220 · Pull Request #14610 · ethereum/go-ethereum

VoR0220 · 2017-06-12T15:13:24Z

Contains moving of certain functions to their own file for unpacking and correction of a type as well as a note about how to handle bool types in the near future.

Signed-off-by: RJ Catalano rj@monax.io

GitCop · 2017-06-12T15:13:26Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: 46578485bc904b6c5612c1963c49c7fa3ce59908
Commit subjects should be kept under 100 characters

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

…ion of name. Signed-off-by: RJ Catalano <rj@monax.io> get rid of some imports Signed-off-by: RJ Catalano <rj@monax.io>

karalabe · 2017-06-13T11:21:11Z

@@ -0,0 +1,220 @@
+// Copyright 2015 The go-ethereum Authors


Please call this file unpack.go.

Ah I see you made it like this because of packing.go. Please change that too to pack.go.

karalabe · 2017-06-13T11:24:39Z

+	}
+}
+
+// todo: this is inefficient for a bool, just look at the last cell, save yourself 32 iterations


Then I guess the proper fix would be a fix instead of a comment ;) Looking at the abi package it seems allZero is only ever used to check if a boolean from a contract is true or false. Perhaps we could get rid of this method altogether?

Not sure of the implications though. Would be nice to have a test suite to validate such a change and whether it breaks anything. What happens if I call a transaction with a bool parameter but set higher order bytes?

If you call a transaction with a bool parameter but set higher order bytes you've broken the ABI encoding :p

Signed-off-by: RJ Catalano <rj@monax.io>

VoR0220 · 2017-06-13T23:46:56Z

@karalabe So I managed to "fix" the boolean function as well. After thinking about what you said, I decided that it is best to iterate through the entire thing if only to make sure that there are no higher order bits being set (and if there is, throw an error). I think the way it's set up now is better. If we want to get REALLY anal about it we can put a switch case for uints to equal 1, 0 or fail.

Signed-off-by: RJ Catalano <rj@monax.io>

VoR0220 · 2017-06-13T23:50:26Z

on second thought, better to be anal than regretful later.

karalabe · 2017-06-14T08:19:47Z

+	case 1:
+		return true, nil
+	default:
+		return false, fmt.Errorf("abi: Improperly encoded boolean value.")


Please don't capitalize error messages and don't add a period at the end. Errors in Go tend to get sometimes concatenated (e.g. if err != nil { return fmt.Errorf("something went wrong: %v", err) }, where capitalization and trailing periods make it weird.

Interesting. Will change.

karalabe · 2017-06-14T08:19:58Z

+			return false, fmt.Errorf("abi: Improperly encoded boolean value.")
+		}
+	}
+	switch uint(b[31]) {


There's no reason to convert this to uint

Yea you're right. Forgot that its a uint8 at the end of the day.

karalabe · 2017-06-14T08:21:33Z

+}
+
+func getBoolValue(b []byte) (bool, error) {
+	for i, byte := range b {


Please don't use byte as a variable, it's a type name. It makes reading code a lot more confusing. Perhaps name the original input b as word and then use b as the iterator.

karalabe · 2017-06-14T08:22:56Z

+
+func getBoolValue(b []byte) (bool, error) {
+	for i, byte := range b {
+		if byte != 0 && i != 31 {


Are you sure the input will contain exactly 32 bytes? This code is quite dangerous because the code does strange logic with the 32. byte, but nowhere is it checked to ensure the 32. byte even exists or whether a 33s exists too. For such logic you'd also need to have a check at the top to ensure the input is 32 bytes long.

With the direction i intend to take this in i can tell you i am splitting the entire set of words into a map of ints to 32 byte words which would then ensure the length and which would nestle this in nicely. But what to do in the interim...

Signed-off-by: RJ Catalano <rj@monax.io>

…e for integers Signed-off-by: RJ Catalano <rj@monax.io>

Signed-off-by: RJ Catalano <rj@monax.io>

…d files. Signed-off-by: RJ Catalano <rj@monax.io>

fjl · 2017-06-21T12:47:57Z

+		input  interface{}
+		output []byte
+	}{
+		{"uint16", uint16(2), pad([]byte{2}, 32, true)},


Please use hex strings for input and output in all tests. IMHO pad(...) etc. are hard to read.

The problem with hex strings is counting the length and im always unsure whether im putting in the proper length of characters.

I have a better idea, we should use a better function that takes in a hex string of varying length, converts it to bytes, pads it to a word and returns the byte slice.

Also. We need a better way of doing the sliceFormatting...it's currently ridiculously confusing to read.

Or better yet, why not just simplify pad() down to two variables, that being input for the bytes input and left for whether its left padded or not ... because currently its a pain to write out the entire hex string and do it correctly for each of the types.

perhaps even throw in a function type in there to append multiple pad statements...hrmmmm....

fjl · 2017-06-21T12:48:40Z

+	improperEncoding := "abi: improperly encoded boolean value"
+	for i, b := range word {
+		if b != 0 && i != 31 {
+			return false, fmt.Errorf(improperEncoding)


Please define errBadBool at package level and use it instead of creating a new error each time.

I don't understand how this benefits the reader of the code nor how it hampers performance? I read this article recently and it changed how I think about package state variables. https://dave.cheney.net/2017/06/11/go-without-package-scoped-variables

fjl · 2017-06-21T12:49:12Z

+		packed []byte
+	}{
+		// Protocol limits
+		{reflect.ValueOf(0), []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}},


Please use hex strings here too.

I actually think this should stay as is. Whats the reasoning for turning this into a hex string?

Signed-off-by: RJ Catalano <rj@monax.io>

fjl · 2017-06-26T12:30:53Z

 )

+var (
+	errBadBool error = errors.New("abi: improperly encoded boolean value")


error type is not needed here

karalabe · 2017-06-26T12:38:08Z

+		}
+
+		if err == nil {
+			// bit of an ugly hack for hash type but I don't feel like finding a proper solution


Signed-off-by: RJ Catalano <rj@monax.io>

VoR0220 requested a review from karalabe June 12, 2017 15:13

VoR0220 force-pushed the smallFixesAndReorganizationOfABI branch from 4657848 to 1bb38aa Compare June 12, 2017 15:16

accounts/abi: reorganizing package and some notes and a quick correct…

a52e1b4

…ion of name. Signed-off-by: RJ Catalano <rj@monax.io> get rid of some imports Signed-off-by: RJ Catalano <rj@monax.io>

VoR0220 force-pushed the smallFixesAndReorganizationOfABI branch from 1bb38aa to a52e1b4 Compare June 12, 2017 16:28

karalabe reviewed Jun 13, 2017

View reviewed changes

RJ Catalano added 3 commits June 13, 2017 17:58

accounts/abi: move file names

07e2f0f

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: fix boolean decode function

c9920a7

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: fix for the array set and for creating a bool

6d9295e

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: be very very very correct

1253f2c

Signed-off-by: RJ Catalano <rj@monax.io>

karalabe suggested changes Jun 14, 2017

View reviewed changes

RJ Catalano added 9 commits June 14, 2017 08:34

accounts/abi: fix up error message and variable names

cb853eb

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: take out unnecessary argument in pack method

ee4ea6c

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: add bool unpack test and add a panic to readBool function

7a861d2

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: fix panic message

98a4ae5

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: change from panic to basic error

f581d54

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: fix nil to false

fc957fa

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: fill out type regex tests and fill with the correct typ…

d168824

…e for integers Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: move packNumbers into pack.go.

65b34f2

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: separation of the testing suite into appropriately name…

02005f7

…d files. Signed-off-by: RJ Catalano <rj@monax.io>

VoR0220 mentioned this pull request Jun 20, 2017

accounts/abi: Better Array/Slice Handling #14664

Closed

fjl self-requested a review June 21, 2017 07:14

fjl requested changes Jun 21, 2017

View reviewed changes

RJ Catalano added 4 commits June 21, 2017 18:21

account/abi: change to hex string tests.

a53d00f

Signed-off-by: RJ Catalano <rj@monax.io>

account/abi: fix up rest of tests to hex

27defc9

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: declare bool at the package level

3afa23f

Signed-off-by: RJ Catalano <rj@monax.io>

accounts/abi: use errors package in the error file.

ef557bb

Signed-off-by: RJ Catalano <rj@monax.io>

fjl requested changes Jun 26, 2017

View reviewed changes

karalabe reviewed Jun 26, 2017

View reviewed changes

accounts/abi: fix ugly hack and fix error type declaration.

441b6d6

Signed-off-by: RJ Catalano <rj@monax.io>

fjl approved these changes Jun 26, 2017

View reviewed changes

karalabe approved these changes Jun 27, 2017

View reviewed changes

karalabe merged commit 5421a08 into ethereum:master Jun 27, 2017

Conversation

VoR0220 commented Jun 12, 2017

Uh oh!

GitCop commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VoR0220 commented Jun 13, 2017

Uh oh!

VoR0220 commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants