Skip to content
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: Implement UniswapV4 Native integration #125

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

tvinagre
Copy link
Contributor

@tvinagre tvinagre commented Dec 17, 2024

Missing:

  • Test

@tvinagre tvinagre changed the title feat(WIP): feat(WIP): Implement UniswapV4 Native integration Dec 17, 2024
@tvinagre tvinagre marked this pull request as draft December 17, 2024 03:39
@tvinagre tvinagre requested a review from zizou0x December 17, 2024 03:39
Copy link
Collaborator

@zizou0x zizou0x left a comment

Choose a reason for hiding this comment

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

Just a few comments but the overall logic looks good

Also why so many fmt changes in others modules?

fee: swap.fee.into(),
})),
})
// Skipped because Donate doesn't seem to affect pool liquidity?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure that it doesn't affect anything we're interest in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah - just changes fee distribution - which we don't care

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but pls, if you want to confirm it, it's always good to have a 2nd check

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll try to have a look, but we're running short in time 😬

}
}

fn get_amount_delta(
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this handles Mint and Burn? It returns a BigInt with the correct sign?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@tvinagre tvinagre force-pushed the tl/uniswap-v4-integration branch 3 times, most recently from 04748bc to 41164d2 Compare December 17, 2024 16:03
@tvinagre tvinagre force-pushed the tl/uniswap-v4-integration branch from 41164d2 to bd07b09 Compare December 17, 2024 16:10
change: ChangeType::Creation.into(),
},
Attribute {
name: "protocol_fees/zero2one".to_string(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to initialize this as 0? We didn't do it for UniV3 - but I think it's weird to Update something that wasn't created before - I'm not sure we enforce it - if we do, we might have a bug on Univ3

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope we don't enforce it, we added the default because when parsing pool state it's better to have a consistent set of attributes than omit some attributes because they were never updated.

Agreed that we should have it on Uniswap v3 though.
Protocol fees isn't really a thing there so it was overlooked because nobody cares about it.

@tvinagre tvinagre force-pushed the tl/uniswap-v4-integration branch from 166a43b to 7c8c9b7 Compare December 18, 2024 04:35
@tvinagre tvinagre self-assigned this Dec 18, 2024
@tvinagre tvinagre marked this pull request as ready for review December 18, 2024 22:38
Copy link
Collaborator

@zizou0x zizou0x left a comment

Choose a reason for hiding this comment

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

Looks good to me, I just have some small code cleaning/polishing requests

The only big thing is the fee that you said are dynamic but we store as static attributes. I think we need to double check how this behaves and how we can properly index (if it needs to be dynamic attribute?)

@@ -0,0 +1,4 @@
[toolchain]
channel = "1.83.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a particular reason to have this fixed and not use stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since every substream is frozen in time, I think it makes more sense to use a fixed version instead of stable

substreams/ethereum-uniswap-v4/rust-toolchain.toml Outdated Show resolved Hide resolved
substreams/ethereum-uniswap-v4/ethereum-uniswap-v4.yaml Outdated Show resolved Hide resolved
substreams/ethereum-uniswap-v4/src/modules/uni_math.rs Outdated Show resolved Hide resolved
type PoolAddress = Vec<u8>;

#[substreams::handlers::map]
pub fn map_protocol_changes(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you change anything here compared to uniswap_v3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Fee naming: protocol_fees/token0 -> protocol_fees/zero2one
  • Match new event names
  • Skip reparsing Initialize
  • Separate lower & upper bits from ProtocolFeeUpdated event

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok perfect, so no change in this map, only adapted event_to_attributes_updates to match USV4 things, right?

Comment on lines +194 to +199
pool_event::Type::ProtocolFeeUpdated(sfp) => {
// Mask to extract the lower 12 bits (0xFFF corresponds to 12 bits set to 1)
let lower_12_bits = sfp.protocol_fee & 0xFFF;

// Shift right by 12 bits and mask again to get the next 12 bits
let upper_12_bits = (sfp.protocol_fee >> 12) & 0xFFF;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we lucky and there is an example of this in Sepolia? Else I'm afraid it will be hard to test this logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to make one during cd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yeah - no events so far

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And - super hard bc only the protocol owner can change this one

Comment on lines +278 to +292
let sign = !liquidity.is_negative();

// Create mask for negative numbers
let mask = if sign { 0u128 } else { u128::MAX };

// Get absolute value of liquidity using XOR and addition
let liquidity = mask ^ mask.wrapping_add_signed(liquidity);

// Convert mask to BigInt (all 1s or all 0s)
let mask = if sign { BigInt::from(0) } else { -BigInt::from(1) };

let amount_1 = get_amount_1_delta(sqrt_ratio_a_x96, sqrt_ratio_b_x96, liquidity, sign)?;

// Apply the mask using XOR and subtraction to restore the sign
Ok((amount_1 ^ &mask) - mask)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can easily remove all bitwise operation there, using just normal functions like abs() and neg() or

Ok(if sign { amount_1 } else { -amount_1 })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I wouldn't like to touch it, as it's an accurate representation of the protocol's math

@tvinagre tvinagre changed the title feat(WIP): Implement UniswapV4 Native integration feat: Implement UniswapV4 Native integration Dec 19, 2024
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