Skip to content

Fix bad geth log decoding #2476

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.

Sign up for GitHub

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

Merged
spooktheducks merged 2 commits into from
Mar 12, 2020
Merged

Fix bad geth log decoding #2476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c251a14
Fix bad geth log decoding
spooktheducks Mar 12, 2020
c1a35b7
Add comment explaining how Geth translates Solidity field names to Go…
spooktheducks Mar 12, 2020
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
10 changes: 5 additions & 5 deletions core/eth/contracts/FluxAggregator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,16 @@ type LogNewRound struct {

type LogRoundDetailsUpdated struct {
PaymentAmount *big.Int
MinAnswerCount *big.Int
MaxAnswerCount *big.Int
RestartDelay *big.Int
Timeout *big.Int
MinAnswerCount uint32
MaxAnswerCount uint32
RestartDelay uint32
Timeout uint32
Address common.Address
}

type LogAnswerUpdated struct {
Current *big.Int
RoundID *big.Int
RoundId *big.Int
Timestamp *big.Int
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.

Maybe keep as RoundID?
https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Copy link
Copy Markdown
Contributor Author

@spooktheducks spooktheducks Mar 12, 2020

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

Address common.Address
}
Expand Down
60 changes: 60 additions & 0 deletions core/eth/contracts/FluxAggregator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,63 @@ func TestFluxAggregatorClient_LatestSubmission(t *testing.T) {
})
}
}

func TestFluxAggregatorClient_DecodesLogs(t *testing.T) {
fa, err := contracts.NewFluxAggregator(nil, common.Address{})
require.NoError(t, err)

newRoundLogRaw := cltest.LogFromFixture(t, "../../services/testdata/new_round_log.json")
var newRoundLog contracts.LogNewRound
err = fa.UnpackLog(&newRoundLog, "NewRound", newRoundLogRaw)
require.NoError(t, err)
require.Equal(t, int64(1), newRoundLog.RoundId.Int64())
require.Equal(t, common.HexToAddress("f17f52151ebef6c7334fad080c5704d77216b732"), newRoundLog.StartedBy)
require.Equal(t, int64(15), newRoundLog.StartedAt.Int64())

type BadLogNewRound struct {
Copy link
Copy Markdown
Contributor

@samsondav samsondav Mar 12, 2020

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

@spooktheducks spooktheducks Mar 12, 2020

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 •
edited
Loading

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

@spooktheducks spooktheducks Mar 12, 2020

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

@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.

Ok as long as it was considered

Copy link
Copy Markdown
Contributor

@samsondav samsondav Mar 12, 2020

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.

RoundID *big.Int
StartedBy common.Address
StartedAt *big.Int
}
var badNewRoundLog BadLogNewRound
err = fa.UnpackLog(&badNewRoundLog, "NewRound", newRoundLogRaw)
require.Error(t, err)

answerUpdatedLogRaw := cltest.LogFromFixture(t, "../../services/testdata/answer_updated_log.json")
var answerUpdatedLog contracts.LogAnswerUpdated
err = fa.UnpackLog(&answerUpdatedLog, "AnswerUpdated", answerUpdatedLogRaw)
require.NoError(t, err)
require.Equal(t, int64(1), answerUpdatedLog.Current.Int64())
require.Equal(t, int64(2), answerUpdatedLog.RoundId.Int64())
require.Equal(t, int64(3), answerUpdatedLog.Timestamp.Int64())

type BadLogAnswerUpdated struct {
Current *big.Int
RoundID *big.Int
Timestamp *big.Int
}
var badAnswerUpdatedLog BadLogAnswerUpdated
err = fa.UnpackLog(&badAnswerUpdatedLog, "AnswerUpdated", answerUpdatedLogRaw)
require.Error(t, err)

roundDetailsUpdatedLogRaw := cltest.LogFromFixture(t, "../../services/testdata/round_details_updated_log.json")
var roundDetailsUpdatedLog contracts.LogRoundDetailsUpdated
err = fa.UnpackLog(&roundDetailsUpdatedLog, "RoundDetailsUpdated", roundDetailsUpdatedLogRaw)
require.NoError(t, err)
require.Equal(t, int64(1), roundDetailsUpdatedLog.PaymentAmount.Int64())
require.Equal(t, uint32(2), roundDetailsUpdatedLog.MinAnswerCount)
require.Equal(t, uint32(3), roundDetailsUpdatedLog.MaxAnswerCount)
require.Equal(t, uint32(4), roundDetailsUpdatedLog.RestartDelay)
require.Equal(t, uint32(5), roundDetailsUpdatedLog.Timeout)

type BadLogRoundDetailsUpdated struct {
Paymentamount *big.Int
MinAnswerCount uint32
MaxAnswerCount uint32
RestartDelay uint32
Timeout uint32
}
var badRoundDetailsUpdatedLog BadLogRoundDetailsUpdated
err = fa.UnpackLog(&badRoundDetailsUpdatedLog, "RoundDetailsUpdated", roundDetailsUpdatedLogRaw)
require.Error(t, err)
}
13 changes: 13 additions & 0 deletions core/eth/geth_copied.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,19 @@ func parseTopics(out interface{}, fields abi.Arguments, topics []common.Hash) er
if !arg.Indexed {
return errors.New("non-indexed field in topic reconstruction")
}

// If Go structs aren't kept correctly in sync with log fields defined in Solidity, this error will be returned.
// The name convention is to remove underscores, capitalize all characters following them, and capitalize the
// first letter of the field:
//
// round_id => RoundId
// roundId => RoundId
// _roundId => RoundId
_, exists := reflect.TypeOf(out).Elem().FieldByName(capitalise(arg.Name))
Comment thread
samsondav marked this conversation as resolved.
Outdated
if !exists {
return errors.Errorf(`can't find matching struct field for log "%T", field "%v" (expected "%v")`, out, arg.Name, capitalise(arg.Name))
}

field := reflect.ValueOf(out).Elem().FieldByName(capitalise(arg.Name))

// Try to parse the topic back into the fields based on primitive types
Expand Down
2 changes: 1 addition & 1 deletion core/services/fluxmonitor/flux_monitor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ func (p *PollingDeviationChecker) respondToAnswerUpdatedLog(log contracts.LogAns
}

func (p *PollingDeviationChecker) respondToRoundDetailsUpdatedLog(log contracts.LogRoundDetailsUpdated) error {
p.roundTimeout = time.Duration(log.Timeout.Int64()) * time.Second
p.roundTimeout = time.Duration(log.Timeout) * time.Second
return nil
}

Expand Down
22 changes: 22 additions & 0 deletions core/services/testdata/answer_updated_log.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"jsonrpc": "2.0",
"method": "eth_subscription",
"params": {
"subscription": "0x4a8a4c0517381924f9838102c5a4dcb7",
"result": {
"logIndex": "0x0",
"transactionIndex": "0x0",
"transactionHash": "0x420de56323893bced814b83f16a94c8ef7f7b6f1e3920a11ec62733fcf82c730",
"blockHash": "0x5e3bd2cc97a68136cead922330e2ec27201420b3eff182875e388474079fcd9e",
"blockNumber": "0xa",
"address": "0x2fCeA879fDC9FE5e90394faf0CA644a1749d0ad6",
"data": "0x0000000000000000000000000000000000000000000000000000000000000003",
"topics": [
"0x0559884fd3a460db3073b7fc896cc77986f16e378210ded43186175bf646fc5f",
"0x0000000000000000000000000000000000000000000000000000000000000001",
"0x0000000000000000000000000000000000000000000000000000000000000002"
],
"type": "mined"
}
}
}
2 changes: 1 addition & 1 deletion core/services/testdata/new_round_log.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"blockHash": "0x5e3bd2cc97a68136cead922330e2ec27201420b3eff182875e388474079fcd9e",
"blockNumber": "0xa",
"address": "0x2fCeA879fDC9FE5e90394faf0CA644a1749d0ad6",
"data": "0x0000000000000000000000000000000000000000000000000000000000000000",
"data": "0x000000000000000000000000000000000000000000000000000000000000000f",
"topics": [
"0x0109fc6f55cf40689f02fbaad7af7fe7bbac8a3d2186600afc7d3e10cac60271",
"0x0000000000000000000000000000000000000000000000000000000000000001",
Expand Down
23 changes: 23 additions & 0 deletions core/services/testdata/round_details_updated_log.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"jsonrpc": "2.0",
"method": "eth_subscription",
"params": {
"subscription": "0x4a8a4c0517381924f9838102c5a4dcb7",
"result": {
"logIndex": "0x0",
"transactionIndex": "0x0",
"transactionHash": "0x420de56323893bced814b83f16a94c8ef7f7b6f1e3920a11ec62733fcf82c730",
"blockHash": "0x5e3bd2cc97a68136cead922330e2ec27201420b3eff182875e388474079fcd9e",
"blockNumber": "0xa",
"address": "0x2fCeA879fDC9FE5e90394faf0CA644a1749d0ad6",
"data": "0x00000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000005",
"topics": [
"0x56800c9d1ed723511246614d15e58cfcde15b6a33c245b5c961b689c1890fd8f",
"0x0000000000000000000000000000000000000000000000000000000000000001",
"0x0000000000000000000000000000000000000000000000000000000000000002",
"0x0000000000000000000000000000000000000000000000000000000000000003"
],
"type": "mined"
}
}
}