Skip to content
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.

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

Fix bad geth log decoding #2476

Merged
merged 2 commits into from
Mar 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Contributor

Choose a reason for hiding this comment

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

Copy link
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

Address common.Address
}
Expand Down
60 changes: 60 additions & 0 deletions core/eth/contracts/FluxAggregator_test.go
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
Collaborator

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
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
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
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
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
Collaborator

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
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))
samsondav marked this conversation as resolved.
Show resolved Hide resolved
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
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
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
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
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"
}
}
}