feat(Scroll): Add syncswap AMM support #7948

zinkkrysty · 2024-05-17T20:39:15Z

Adds support for decoding syncswap transactions (swap, liquidity add/remove). Syncswap is a uniswap V2-like protocol, so a common evm module was added for decoding uniswap V2-like protocols. The intention is to use this for uniswap v2 and sushiswap, however that would have made the scope of this PR too big.

https://syncswap.gitbook.io/api-documentation

codecov · 2024-05-17T20:44:25Z

Codecov Report

Attention: Patch coverage is 85.71429% with 17 lines in your changes missing coverage. Please review.

Project coverage is 53.37%. Comparing base (b698e9d) to head (9f11649).
Report is 1 commits behind head on develop.

Files	Patch %	Lines
...chen/chain/evm/decoding/uniswap_v2_like/decoder.py	82.97%	6 Missing and 2 partials ⚠️
...lchen/chain/evm/decoding/uniswap_v2_like/common.py	83.33%	5 Missing ⚠️
...tkehlchen/chain/scroll/modules/syncswap/decoder.py	85.71%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #7948       +/-   ##
============================================
- Coverage    80.51%   53.37%   -27.15%     
============================================
  Files         1224     1681      +457     
  Lines       108648   167177    +58529     
  Branches     13154    13678      +524     
============================================
+ Hits         87480    89228     +1748     
- Misses       18842    75616    +56774     
- Partials      2326     2333        +7

Flag	Coverage Δ
backend	`80.76% <85.71%> (-0.02%)`	⬇️
frontend_integration	`54.65% <ø> (-4.81%)`	⬇️
frontend_unit	`38.69% <ø> (-40.64%)`	⬇️

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.

rotkehlchen/chain/ethereum/modules/sushiswap/constants.py

rotkehlchen/tests/unit/decoders/test_syncswap.py

rotkehlchen/chain/evm/decoding/uniswap_v2_like/common.py

OjusWiZard · 2024-05-21T09:54:11Z

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

+log = RotkehlchenLogsAdapter(logger)
+
+
+class UniswapV2LikeDecoder(DecoderInterface):


is this class being used anywhere else except syncswap? It seems like a right abstraction, but maybe not needed right now?

it's not used, but it would make sense to use it for chain/ethereum/modules/uniswap/v2/decoder.py and rotkehlchen/chain/ethereum/modules/sushiswap/decoder.py

Though I am unsure if doing the work in the post_decoding_rules as I am doing here is better or worse than using normal decoding rules. It's just that for syncswap the order of the events (I think for the deposit/withdraw) was reversed so I needed to access the decoded transfer events at the beginning.

If the other works without post decoding and this one doesn't then probably the logic of syncswap could be different, but also we can't know for sure without trying this class on them. So for just syncswap right now, I would get a second opinion from yabir and/or lefteris also.

I know that you can't always abstract similar logic and trying to fit everything in one box is not always feasible, and can lead to performance problems. However I also think that a good principle to strive for is to abstract as much as possible, so that the common logic for decoding swaps would work in principle for any AMM, so that we can add add many AMMs as possible using as little code as possible. In some cases (such as for any AMM based on Uniswap V2 which is very popular), we might want to resort to the common denominator (in this case using post-decoding) if that would work for most AMM contracts.

post decoding rules is the worst type of rule for performance. I would not use it in an abstraction.

OjusWiZard · 2024-05-21T10:01:57Z

also please add a changelog entry and rebase the PR on the latest develop branch

zinkkrysty · 2024-06-05T18:11:35Z

@OjusWiZard sorry for the long delay 😬. I rebased now (also the test-caching branch) and addressed most comments. I am not in a hurry to get this out. If you think the design of UniswapV2LikeDecoder and the common methods could be improved, I can take a stab at that - but will need some directions.

OjusWiZard

heyy @zinkkrysty, some very small requests

rotkehlchen/chain/ethereum/modules/uniswap/v2/constants.py

rotkehlchen/chain/evm/decoding/uniswap_v2_like/common.py

OjusWiZard · 2024-06-06T12:45:04Z

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

+log = RotkehlchenLogsAdapter(logger)
+
+
+class UniswapV2LikeDecoder(DecoderInterface):


If the other works without post decoding and this one doesn't then probably the logic of syncswap could be different, but also we can't know for sure without trying this class on them. So for just syncswap right now, I would get a second opinion from yabir and/or lefteris also.

Adds support for decoding syncswap transactions (swap, liquidity add/remove). Syncswap is a uniswap V2-like protocol, so a common evm module was added for decoding uniswap V2-like protocols.

zinkkrysty · 2024-06-06T20:17:32Z

@OjusWiZard thanks again for the review. I addressed all the suggestions

LefterisJP

Lots of comments. Also needs rebase and adjusting changelog. In general make small PRs, succint and keeping consistency with the rest of the codebase.

Abstractions can come later. Otherwise it's impossible to review and extremely time consuming to merge.

LefterisJP · 2024-06-09T21:21:03Z

rotkehlchen/chain/evm/decoding/uniswap_v2_like/common.py


-    if token0 is None or token1 is None:
+    if token0 is None and token1 is None:
+        log.error(f'None of the tokens with amounts {amount0_raw} and {amount1_raw} were found in the event log')  # noqa: E501


Add token addresses and chain name here. You can get chain name from evm inquirer. Logs should be as informative as possible since it's the only thing we have during debugging

LefterisJP · 2024-06-09T21:23:52Z

rotkehlchen/chain/evm/decoding/uniswap_v2_like/common.py

        factory_address: ChecksumEvmAddress,
        init_code_hash: str,
        tx_hash: EVMTxHash,
+        compute_pool_address: Callable[[EvmToken, EvmToken, ChecksumEvmAddress, str], ChecksumEvmAddress] = _compute_uniswap_v2_like_pool_address,  # noqa: E501
+        weth_asset: Asset = A_WETH,


Why are you giving this as a default argument. Please don't. There should be no default argument here as this only works for ethereum mainnet.

Also pass it as a resolved EVM token

Suggested change

weth_asset: Asset = A_WETH,

weth_token: EVMToken,

LefterisJP · 2024-06-09T21:25:20Z

rotkehlchen/chain/evm/decoding/uniswap_v2_like/common.py

+        asset_0 = resolved_eth
+        token0 = EvmToken(weth_asset.identifier)
+    if token1 is None:
+        asset_1 = resolved_eth
+        token1 = EvmToken(weth_asset.identifier)


If you pass it resolved as I suggested above then no need to do the constructor here

Suggested change

asset_0 = resolved_eth

token0 = EvmToken(weth_asset.identifier)

if token1 is None:

asset_1 = resolved_eth

token1 = EvmToken(weth_asset.identifier)

asset_0, token0 = resolved_eth, weth_token

if token1 is None:

asset_1, token1 = resolved_eth, weth_token

LefterisJP · 2024-06-09T21:27:01Z

rotkehlchen/chain/evm/decoding/uniswap_v2_like/common.py

    )
    if pool_address != target_pool_address:  # we didn't find the correct pool
+        log.debug(f'Pool address {pool_address} does not match target pool address {target_pool_address}')  # noqa: E501


Why debug? Isn't this an error? When can this happen? Again here you also need to write the chain name.

LefterisJP · 2024-06-09T21:34:28Z

rotkehlchen/chain/evm/decoding/uniswap_v2_like/common.py

-        token1=token1,
-        factory_address=factory_address,
-        init_code_hash=init_code_hash,
+    pool_address = compute_pool_address(


It's actually possible to do it. The only reason typing fails here is because you use callable. Read up on how Protocol works.

A quick example that typechecks.

from typing import Callable, Protocol def foo(a: int, b: str) -> int: return 5 def boo(a: int, b:str) -> int: return 10 class CallableType(Protocol): def __call__(self, a: int, b: str) -> int: ... def ourfunc(function: CallableType) -> int: return function(a=3, b='foo') print(ourfunc(foo))

LefterisJP · 2024-06-09T21:43:27Z

rotkehlchen/chain/scroll/modules/syncswap/decoder.py

+# The init code hash is looked up using the RPC method debug_traceTransaction() from the pool initialization transaction  # noqa: E501
+# The pool_bytecode is the input of the CREATE2 call, then we pass it to Web3.keccak(hexstring_to_bytes(pool_bytecode))  # noqa: E501


Don't put noqa: E501 in comments unless it's absolutely needed. Just break into more lines.

Suggested change

# The init code hash is looked up using the RPC method debug_traceTransaction() from the pool initialization transaction # noqa: E501

# The pool_bytecode is the input of the CREATE2 call, then we pass it to Web3.keccak(hexstring_to_bytes(pool_bytecode)) # noqa: E501

# The init code hash is looked up using the RPC method debug_traceTransaction() from the

# pool initialization transaction. The pool_bytecode is the input of the CREATE2 call, then

# we pass it to Web3.keccak(hexstring_to_bytes(pool_bytecode))

LefterisJP · 2024-06-09T21:44:11Z

rotkehlchen/chain/scroll/modules/syncswap/decoder.py

+                SYNCSWAP_STABLE_POOL_FACTORY_ADDRESS: SYNCSWAP_STABLE_INIT_CODE_HASH,
+            },
+        )
+        self.weth_asset = A_WETH_SCROLL.resolve_to_evm_token()


If you needed to use inheritance here this shoudl simply have beeen part of super().init().

LefterisJP · 2024-06-09T21:44:46Z

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

+        else:
+            return
+
+        if self.weth_asset is None:


this if does not need to exist. It's always true. If you use inheritance properly.

LefterisJP · 2024-06-09T21:45:32Z

rotkehlchen/tests/unit/decoders/test_syncswap.py

+        ),
+        EvmEvent(


Here and in the rest of the file.

Suggested change

),

EvmEvent(

), EvmEvent(

LefterisJP · 2024-06-09T21:46:32Z

rotkehlchen/tests/unit/decoders/test_syncswap.py

+    timestamp = TimestampMS(1713530626000)
+    fee = FVal('0.00024694228625118')
+    amount_received_usdc = FVal('36.508481')
+    amount_sent_eth = FVal('0.01180498459310175')


Here and in the rest of the file. Please keep style consistent.

Suggested change

timestamp = TimestampMS(1713530626000)

fee = FVal('0.00024694228625118')

amount_received_usdc = FVal('36.508481')

amount_sent_eth = FVal('0.01180498459310175')

timestamp, fee, amount_received_usdc, amount_sent_eth = TimestampMS(.....

zinkkrysty mentioned this pull request May 17, 2024

Test VCRs for syncswap on scroll rotki/test-caching#44

Merged

zinkkrysty force-pushed the feature-syncswap-scroll branch from 7baa334 to 99ce5a7 Compare May 17, 2024 21:03

OjusWiZard reviewed May 21, 2024

View reviewed changes

zinkkrysty force-pushed the feature-syncswap-scroll branch from 99ce5a7 to fad8606 Compare June 5, 2024 17:57

zinkkrysty force-pushed the feature-syncswap-scroll branch from fad8606 to ab354dc Compare June 5, 2024 18:13

OjusWiZard reviewed Jun 6, 2024

View reviewed changes

feat(Scroll): Add syncswap AMM support

9f11649

Adds support for decoding syncswap transactions (swap, liquidity add/remove). Syncswap is a uniswap V2-like protocol, so a common evm module was added for decoding uniswap V2-like protocols.

zinkkrysty force-pushed the feature-syncswap-scroll branch from ab354dc to 9f11649 Compare June 6, 2024 20:16

OjusWiZard added the ready for final review Backend PR ready to be reviewed by great Lefteris label Jun 7, 2024

LefterisJP reviewed Jun 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Scroll): Add syncswap AMM support #7948

feat(Scroll): Add syncswap AMM support #7948

zinkkrysty commented May 17, 2024

codecov bot commented May 17, 2024 •

edited

OjusWiZard May 21, 2024

zinkkrysty Jun 5, 2024

OjusWiZard Jun 6, 2024

zinkkrysty Jun 6, 2024

LefterisJP Jun 9, 2024

OjusWiZard commented May 21, 2024

zinkkrysty commented Jun 5, 2024

OjusWiZard left a comment

OjusWiZard Jun 6, 2024

zinkkrysty commented Jun 6, 2024

LefterisJP left a comment

LefterisJP Jun 9, 2024

LefterisJP Jun 9, 2024

LefterisJP Jun 9, 2024

LefterisJP Jun 9, 2024

LefterisJP Jun 9, 2024

LefterisJP Jun 9, 2024

LefterisJP Jun 9, 2024

LefterisJP Jun 9, 2024

LefterisJP Jun 9, 2024

LefterisJP Jun 9, 2024

		log = RotkehlchenLogsAdapter(logger)


		class UniswapV2LikeDecoder(DecoderInterface):

		# The init code hash is looked up using the RPC method debug_traceTransaction() from the pool initialization transaction # noqa: E501
		# The pool_bytecode is the input of the CREATE2 call, then we pass it to Web3.keccak(hexstring_to_bytes(pool_bytecode)) # noqa: E501

feat(Scroll): Add syncswap AMM support #7948

Are you sure you want to change the base?

feat(Scroll): Add syncswap AMM support #7948

Conversation

zinkkrysty commented May 17, 2024

codecov bot commented May 17, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OjusWiZard commented May 21, 2024

zinkkrysty commented Jun 5, 2024

OjusWiZard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zinkkrysty commented Jun 6, 2024

LefterisJP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 17, 2024 •

edited