-
-
Notifications
You must be signed in to change notification settings - Fork 502
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
feat(Scroll): Add syncswap AMM support #7948
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7baa334
to
99ce5a7
Compare
log = RotkehlchenLogsAdapter(logger) | ||
|
||
|
||
class UniswapV2LikeDecoder(DecoderInterface): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this class being used anywhere else except syncswap? It seems like a right abstraction, but maybe not needed right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
post decoding rules is the worst type of rule for performance. I would not use it in an abstraction.
also please add a changelog entry and rebase the PR on the latest develop branch |
99ce5a7
to
fad8606
Compare
@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 |
fad8606
to
ab354dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heyy @zinkkrysty, some very small requests
log = RotkehlchenLogsAdapter(logger) | ||
|
||
|
||
class UniswapV2LikeDecoder(DecoderInterface): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ab354dc
to
9f11649
Compare
@OjusWiZard thanks again for the review. I addressed all the suggestions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
weth_asset: Asset = A_WETH, | |
weth_token: EVMToken, |
asset_0 = resolved_eth | ||
token0 = EvmToken(weth_asset.identifier) | ||
if token1 is None: | ||
asset_1 = resolved_eth | ||
token1 = EvmToken(weth_asset.identifier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you pass it resolved as I suggested above then no need to do the constructor here
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 |
) | ||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why debug? Isn't this an error? When can this happen? Again here you also need to write the chain name.
token1=token1, | ||
factory_address=factory_address, | ||
init_code_hash=init_code_hash, | ||
pool_address = compute_pool_address( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't put noqa: E501 in comments unless it's absolutely needed. Just break into more lines.
# 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)) |
SYNCSWAP_STABLE_POOL_FACTORY_ADDRESS: SYNCSWAP_STABLE_INIT_CODE_HASH, | ||
}, | ||
) | ||
self.weth_asset = A_WETH_SCROLL.resolve_to_evm_token() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you needed to use inheritance here this shoudl simply have beeen part of super().init().
else: | ||
return | ||
|
||
if self.weth_asset is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this if does not need to exist. It's always true. If you use inheritance properly.
), | ||
EvmEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in the rest of the file.
), | |
EvmEvent( | |
), EvmEvent( |
timestamp = TimestampMS(1713530626000) | ||
fee = FVal('0.00024694228625118') | ||
amount_received_usdc = FVal('36.508481') | ||
amount_sent_eth = FVal('0.01180498459310175') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in the rest of the file. Please keep style consistent.
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(..... |
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