hodgerpodger / staketaxcsv

python repo to create blockchain CSVs
MIT License
252 stars 119 forks source link

[Juno] Fix crash when no action present in event logs #321

Closed zapnap closed 11 months ago

zapnap commented 1 year ago

It appears that some Juno transaction event logs don't include an action (for whatever reason), which causes the report to crash.

zapnap commented 1 year ago

Also looks like IBC transfers are being labeled as tx_type _UNKNOWN for some reason... 🤔

zapnap commented 1 year ago

I'm sure that I'm grossly misunderstanding this logic, but it seems like the root of my issue is in the common ibc processor code where it's assumed that the message length should equal the number of events? @hodgerpodger can you educate me here? 🙏

for i in range(len(elem["logs"])):
    message = elem["tx"]["body"]["messages"][i]
    log = elem["logs"][i]

    if customMsgInfo:
        msginfo = customMsgInfo(wallet_address, i, message, log, lcd_node, ibc_addresses)
    else:
        msginfo = MsgInfoIBC(wallet_address, i, message, log, lcd_node, ibc_addresses)
    msgs.append(msginfo)
zapnap commented 1 year ago

Note to self: This seems pretty typical for an IBC received event... I'm noticing that there are 3 messages in the event log, only one of which has an associated action

(just leaving this here as an example)

hodgerpodger commented 1 year ago

Thanks for the contribution! Internal tests passed so that's good. I'll have more time to look at it later when I finish some current work but general unwritten guideline I'm nervous about:

I never want the parser to parse any transaction incorrectly with confidence (should always either parse correctly, treat as _UNKNOWN, or crash).

So I'm not sure about the changes to api_rpc.py and processor.py -- I wonder if these changes leave the door open for silently omitting transactions, etc. (as opposed to treating as _UNKNOWN, crashing, or parsing correctly) in non-Juno transactions.

Ideally, it'd be better to somehow handle specifically what you see in these Juno transactions (as opposed to catch all exceptions in common ibc code). But I don't have any specific suggestion how to exactly do this until I can spend more time on it more closely.

Also, it's been a little while since I've been in this code so my memory might be a little off. So I certainly might not understand all the details--hopefully I can look at it closely in the near future if you might be dubious of this suggestion.

zapnap commented 1 year ago

@hodgerpodger all this PR does is prevent the script from crashing and ignore events from the event log that are not "properly" formed messages. No changes to the way anything is classified. The note about _UNKNOWN events was more of a note that there's still some issues in the Juno logic that need to be sorted out -- I didn't update any of that logic.

At this point the script runs reliably on every account I've pointed it at (was crashing on all of them previously) but several events still end up being flagged as unknown. Which, to your point, is still better than it being misclassified!

hodgerpodger commented 11 months ago

Similar fix with https://github.com/hodgerpodger/staketaxcsv/commit/9add21742b0fa9d3621aea1027fb1d91d90134fc just now (inspired by this PR). Thanks for the patience and work.

zapnap commented 11 months ago

👍