Added support for decoding Lido contracts events on ethereum network - Part 1 - Submit topic

[gianluca-pub-dev]

Added support for Lido contract on eth blockchain for the following events:

• staking submitting ETH to stETH contract, and receiving stETH in return

temporarly removed from the PR:

- staking transferring ETH to wstETH contract and receiving wstETH in return, tracking the calculated wstETH ratio at the time of the staking
- wrapping stETH receiving wstETH, tracking the calculated wstETH ratio at the time of the wrapping
- unwrapping wstETH receiving stETH, , tracking the calculated wstETH ratio at the time of the unwrapping

https://docs.lido.fi/contracts/lido/#submit-1
https://docs.lido.fi/contracts/wsteth
https://docs.lido.fi/contracts/lido/#transfershares

This maybe covers the backend side of B.2 point from issue: #3433

Checklist

• The PR modified the frontend, and updated the user guide to reflect the changes.

[gianluca-pub-dev]

Rotki team: I would like to we discuss with you about how to map the lido events, because it is a staking, but users receive tokens in return that have a deep market, and often they are exchanged without returning the tokens to the Lido contract, so I am not sure how to map (I have never used the STAKING event type for this reason)

[LefterisJP]

I would map it with same events as we do in compound, aave and other tokens. Not as staking. So same events type/subtypes as we use in compound, aave etc for wrapping and unwrapping a token

[codecov]

Codecov Report

Attention: Patch coverage is 87.09677% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 80.69%. Comparing base (f30ee51) to head (03e52da).
Report is 5 commits behind head on develop.

Files	Patch %	Lines
rotkehlchen/chain/ethereum/modules/lido/decoder.py	84.90%	5 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #7949      +/-   ##
===========================================
- Coverage    80.74%   80.69%   -0.06%     
===========================================
  Files         1197     1202       +5     
  Lines       108493   108741     +248     
  Branches     12990    13029      +39     
===========================================
+ Hits         87608    87744     +136     
- Misses       18616    18703      +87     
- Partials      2269     2294      +25     

Flag	Coverage Δ
backend	80.85% <87.09%> (-0.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[LefterisJP]

Hey @gianluca-pub-dev thanks for the time you put at this PR!

I left a lot of comments but now after 40 mins of trying to understand what you are trying to do I have to take a step back as I can't dedicate more time at this.

I tried to leave some comments to explain how decoders are supposed to work.

My advice is the following.

1. Stash the current PR
2. Read and understand all the comments/feedback
3. Make a much smaller PR for just a single function. Say only to deposit ETH for stETH, along with a test. Nothing else. Only that. A really small and clean PR. And let's discusss for this single method first and try to see how to do it properly.

The point to understand is how decoders work. Please follow the code of the main logic of how decode_transaction works and what is called and in what order. Also what kind of rules we have. The most normal ones, based on addresses of logs, then on input data, then post decoding etc.

But most importantly this should help you understand what gets called and how often. To see why what you wrote here suboptimal and why it would slow down the decoder system a lot.

[LefterisJP]

Also a test that failed: rotkehlchen/tests/unit/decoders/test_main.py::test_decoders_initialization needs super small tweak. Whenever you add a new decoder file you need to tweak this in two places to take the new decoder into account. Should be super easy to fix.

[gianluca-pub-dev]

Hey @gianluca-pub-dev thanks for the time you put at this PR!

I left a lot of comments but now after 40 mins of trying to understand what you are trying to do I have to take a step back as I can't dedicate more time at this.

I tried to leave some comments to explain how decoders are supposed to work.

My advice is the following.

1. Stash the current PR

2. Read and understand all the comments/feedback

3. Make a much smaller PR for just a single function. Say only to deposit ETH for stETH, along with a test. Nothing else. Only that. A really small and clean PR. And let's discusss for this single method first and try to see how to do it properly.

The point to understand is how decoders work. Please follow the code of the main logic of how decode_transaction works and what is called and in what order. Also what kind of rules we have. The most normal ones, based on addresses of logs, then on input data, then post decoding etc.

But most importantly this should help you understand what gets called and how often. To see why what you wrote here suboptimal and why it would slow down the decoder system a lot.

Thank you @LefterisJP for the time you spent for the review. I will take care of all the comments.

[LefterisJP]

hey @gianluca-pub-dev can't have multple threads in the review. It's impossible to follow. I asked a simple thing in the last review. Reduce the PR to just this and nothing else.

Make a much smaller PR for just a single function. Only to deposit ETH for stETH, along with a test. Nothing else. Only that. A really small and clean PR. And let's discusss for this single method first and try to see how to do it properly.

Now for how to do it properly just use the single post decoding rule I mentioned. You start from the zero -> tracked address stETH transfer.

Then you can use whatever logic to find the ETH itnernal transfer of amount + difference. And that's it. You got both events.

Just do a PR with this single simple thing please. Force push on this one.

[gianluca-pub-dev]

hey @gianluca-pub-dev can't have multple threads in the review. It's impossible to follow. I asked a simple thing in the last review. Reduce the PR to just this and nothing else.

Make a much smaller PR for just a single function. Only to deposit ETH for stETH, along with a test. Nothing else. Only that. A really small and clean PR. And let's discusss for this single method first and try to see how to do it properly.

Now for how to do it properly just use the single post decoding rule I mentioned. You start from the zero -> tracked address stETH transfer.

Then you can use whatever logic to find the ETH itnernal transfer of amount + difference. And that's it. You got both events.

Just do a PR with this single simple thing please. Force push on this one.

Ok, I will reduce the size of the PR with only the eth submit for helping the review. But I am not sure that the solution of searching the stETH transfer in post_decoding rules is better as performance, since the function would be run for each transaction

rotki/rotkehlchen/chain/evm/decoding/decoder.py

Line 480 in 6644849

events = self.run_all_post_decoding_rules(

.

I am going to wait some time before making this modification, so to give more time for taking a final decision.

Continuing here also #7949 (comment) conversation, we have 2 choices in my opinion:

• As I did until now in this PR, run the decoding for all the events related to stETH and wstETH tokens, but exiting if they don't involve the topics we are interested on.
• Move the logic in only post_decoding rules, but those would run for each transaction, iterating all the decoded events of each transaction.

[LefterisJP]

So @gianluca-pub-dev the post decoding events have limitations. They run only if certain counterparties or certain input data have been found. So you would need to also set those conditions. Specifically here it being stETH transfer. Runnign always would indeed be bad.

But I remembered another decoding rule that is probably a perfect fit here that we made for such situations.

As I said above you need to find the stETH transfer from 0x0..00 to the tracked address right? So this is essentially augmenting a normal transfer. This is something we already do for some transfers via _enrich_protocol_tranfers logic.

All decoders also have the possiility of adding a token enricher rule. This is exactly what we need here. So add some extra logic to run once the token transfer is made and only then.

An example for this is already done in xdai bridge logic:

rotki/rotkehlchen/chain/ethereum/modules/xdai_bridge/decoder.py

Lines 46 to 71 in 6644849

	def _maybe_enrich_dai_transfers(self, context: EnricherContext) -> TransferEnrichmentOutput:
	"""Unfortunately not all xDAI bridging emits the event we match.
	So we need this special matching of DAI transfers to the bridge address
	in order to find all bridging events.
	"""
	if (
	context.event.event_type == HistoryEventType.SPEND and
	context.event.event_subtype == HistoryEventSubType.NONE and
	context.event.asset == A_DAI and
	context.event.address == BRIDGE_ADDRESS
	):
	context.event.event_type = HistoryEventType.DEPOSIT
	context.event.event_subtype = HistoryEventSubType.BRIDGE
	context.event.notes = f'Bridge {context.event.balance.amount} DAI from Ethereum to Gnosis via Gnosis Chain bridge' # noqa: E501
	context.event.counterparty = CPT_GNOSIS_CHAIN
	context.event.address = BRIDGE_ADDRESS
	return TransferEnrichmentOutput(matched_counterparty=CPT_GNOSIS_CHAIN)
	return FAILED_ENRICHMENT_OUTPUT
	# -- DecoderInterface methods
	def enricher_rules(self) -> list[Callable]:
	return [
	self._maybe_enrich_dai_transfers,
	]

[gianluca-pub-dev]

.

Ok @LefterisJP , but _enrich_protocol_tranfers would be run for each token transfer regardless of the token type, right?

rotki/rotkehlchen/chain/evm/decoding/decoder.py

Line 947 in 6644849

enrichment_output = self._enrich_protocol_tranfers(

And with your proposal, inside its logic I should check if the token transfer is the one we are insterested, and then proceed.

But this would not work for mapping the stETH wrap or unwrap in wstETH, because 2 token transfers are involved on that operation, and both token transfers would not be already decoded at that time since _enrich_protocol_tranfers is run in the context of processing one token transfer. And an ActionItem could not be used in this case without iterating the tx_log as I proposed, because the wstETH ratio is not known.

Also I need to say that I arrived to propose this PR after looking at all the options I found in rotki code, including enricher rules. It still seems to me that the best option is the iteration of tx_log for matching the operations, after checking that the topic is the one we are interested on. But I am open to listen any feedback.

[LefterisJP]

hey @gianluca-pub-dev I left another review of this single action PR.

And with your proposal, inside its logic I should check if the token transfer is the one we are insterested, and then proceed.
But this would not work for mapping the stETH wrap or unwrap in wstETH, because 2 token transfers are involved on that operation, and both token transfers would not be already decoded at that time since _enrich_protocol_tranfers is run in the context of processing one token transfer. And an ActionItem could not be used in this case without iterating the tx_log as I proposed, because the wstETH ratio is not known.
Also I need to say that I arrived to propose this PR after looking at all the options I found in rotki code, including enricher rules. It still seems to me that the best option is the iteration of tx_log for matching the operations, after checking that the topic is the one we are interested on. But I am open to listen any feedback.

So the reason I suggested enrich transfer is because your current approach:

1. It will run for all stETH related events. Approvals, transfers, deposits etc. Anything that will touch this contract's address.
2. You are iterating all logs once to find the stETH ERC20 transfer due to the amount difference
3. You are then iterating all decoded events to find the internal ETH transfer
4. Then your are using the action item functionality which will match the decoded ERC20 transfer in the future

This is all a lot of things to do and quite often as stETH is one of the most used tokens in ethereum mainnet.

Alternatively my suggestion was to use enrich transfer due to the fact that with just a simple if condition addition to all ERC20 transfers you can go to the logic you want. So it would just add a single if condition.

---

But after looking at the smaller PR (there is a very good reason we ask for small PRs so they are easy to understand and review) I see some more improvements which can make toyour suggestion even more preferrable to anything else.

I left them in the review itsef. Please talk to me and/or other devs in dev-support channel in discord if something is unclear

[LefterisJP]

Left some more comments. Also pushed your VCR PR to upstream so tests run properly.

[LefterisJP]

LGTM. Left 2 small nitpicks that can just be done in next PR/s

[LefterisJP]

This is constant. No need to resolve (as resolution is DB roundtrip)

Suggested change

	notes=f'Submit {amount_deposited} {A_ETH.symbol_or_name()} to Lido',
	notes=f'Submit {amount_deposited} ETH to Lido',

[LefterisJP]

sam

Suggested change

	notes=f'Receive {amount_minted} {A_STETH.symbol_or_name()} in exchange for the deposited {A_ETH.symbol_or_name()}', # noqa: E501
	notes=f'Receive {amount_minted} stETH in exchange for the deposited ETH', # noqa: E501

[rotkibot]

rotki/test-caching/tree/feat/lido_support was successfully merged