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

Correctly parse uint8 values in contract watcher #139

Merged
merged 1 commit into from
Sep 23, 2019

Conversation

rmulhol
Copy link
Contributor

@rmulhol rmulhol commented Sep 21, 2019

  • calling string(uint8) => insert errors on numeric columns

- calling string(uint8) => insert errors on numeric columns
Copy link
Contributor

@elizabethengelman elizabethengelman left a comment

Choose a reason for hiding this comment

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

💯 :shipit:

@@ -89,9 +89,9 @@ func (c *Converter) Convert(logs []gethTypes.Log, event types.Event, headerID in
if len(b) == 32 {
seenHashes = append(seenHashes, common.HexToHash(strValues[fieldName]))
}
case byte:
b := input.(byte)
strValues[fieldName] = string(b)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any need to keep this case byte in here as well? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so - byte is an alias for uint8, so I could probably leave that as the name for this case if that seems clearer

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. I like this case as uint8, it makes the type check on the next line make more sense to me.

Copy link
Collaborator

@i-norden i-norden left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for making these patches!

@rmulhol rmulhol merged commit dc30099 into staging Sep 23, 2019
@rmulhol rmulhol deleted the contract-watcher-uint8 branch September 23, 2019 20:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants