Skip to content

Fix bad geth log decoding#2476

Merged
spooktheducks merged 2 commits intodevelopfrom
171761388-bug/fix-bad-geth-log-decoding
Mar 12, 2020
Merged

Fix bad geth log decoding#2476
spooktheducks merged 2 commits intodevelopfrom
171761388-bug/fix-bad-geth-log-decoding

Conversation

@spooktheducks
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is it bad ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In Solidity, the round ID field is roundId. Geth's Go code expects this to translate to RoundId, not RoundID, and fails to find the field if it doesn't.

Copy link
Copy Markdown
Contributor

@felder-cl felder-cl Mar 12, 2020

Choose a reason for hiding this comment

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

ethereum/go-ethereum#16648 <- does go-eth support these 'abi' tags?

i.e.

type MyEventParameters struct {
     Value1 *big.Int `abi:"value"`
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Only on non-indexed fields. I plan to rework their decoding logic in a subsequent PR to add this feature for indexed fields.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok as long as it was considered

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we pls have a comment explaining this. Not obvious.

Comment thread core/eth/geth_copied.go Outdated
@felder-cl
Copy link
Copy Markdown
Contributor

Might be good to have a PR description and/or comment to help introduce context

Comment thread core/eth/contracts/FluxAggregator.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, that was the whole issue in the first place -- Geth's code expected a lowercase final d

RyanRHall
RyanRHall previously approved these changes Mar 12, 2020
@spooktheducks spooktheducks dismissed samsondav’s stale review March 12, 2020 18:06

Changes made and this is urgent

@spooktheducks spooktheducks force-pushed the 171761388-bug/fix-bad-geth-log-decoding branch from cedcd85 to c1a35b7 Compare March 12, 2020 18:07
@RyanRHall RyanRHall self-requested a review March 12, 2020 18:26
Copy link
Copy Markdown
Contributor

@samsondav samsondav left a comment

Choose a reason for hiding this comment

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

LGTM just one tiny comment request

@spooktheducks spooktheducks merged commit 8b543fb into develop Mar 12, 2020
@spooktheducks spooktheducks deleted the 171761388-bug/fix-bad-geth-log-decoding branch March 12, 2020 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants