Skip to content

Update imports to match vdb#266

Merged
rmulhol merged 1 commit intostagingfrom
updated-geth-imports
Aug 13, 2020
Merged

Update imports to match vdb#266
rmulhol merged 1 commit intostagingfrom
updated-geth-imports

Conversation

@rmulhol
Copy link
Copy Markdown
Contributor

@rmulhol rmulhol commented Aug 11, 2020

  • new Geth version

@rmulhol rmulhol force-pushed the updated-geth-imports branch 2 times, most recently from b47fad0 to 8aa6ee2 Compare August 11, 2020 22:01
Copy link
Copy Markdown
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.

🙏

func (t Transformer) ToModels(_ string, logs []core.EventLog, _ *postgres.DB) ([]event.InsertionModel, error) {
var models []event.InsertionModel
for _, log := range logs {
var entity NewCdpEntity
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.

Trying to wrap my head around this change... looks like we're not using UnpackLog anymore, and doing it manually now. Is this just because UnpackLog isn't able to unpack the ID into big.Int anymore? Wondering if this is going to affect other transformers too?

Copy link
Copy Markdown
Contributor Author

@rmulhol rmulhol Aug 12, 2020

Choose a reason for hiding this comment

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

The issue seemed to be, as far as I can tell, that UnpackLog doesn't deal well with cases where all of the payload is in the topics and not the data - but the data payload is non-empty. We were hitting this error: https://github.com/ethereum/go-ethereum/blob/master/accounts/abi/argument.go#L95

On a closer look, though, it seems like the problem might be that our test data erroneously includes a non-empty data payload - not seeing any data for similar logs on Etherscan. Thinking maybe it makes sense to clean up the test data, but I'm on the fence about whether we should restore toEntities (since parsing this payload is fairly trivial without that tooling) 🤔

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.

Yeah, that totally makes to me to stay with not using UnpackLog for this one. Also good news that it may be a test data issue - I was thinking that it would be surprising that nothing else would have been affected by this change.

This was referenced Aug 12, 2020
- new Geth version
@rmulhol rmulhol force-pushed the updated-geth-imports branch from 8aa6ee2 to 6394db7 Compare August 13, 2020 19:57
@rmulhol rmulhol merged commit eb5a56b into staging Aug 13, 2020
@rmulhol rmulhol deleted the updated-geth-imports branch August 13, 2020 20:14
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.

2 participants