-
Notifications
You must be signed in to change notification settings - Fork 37
feat: Implement Ethereum Rocketpool integration #321
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
base: main
Are you sure you want to change the base?
Conversation
zizou0x
left a comment
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.
Just had a quick look at the overall logic and it looks good! I'll let @louise-poole review the protocol related things
substreams/ethereum-rocketpool/src/modules/4_map_protocol_changes.rs
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,198 @@ | |||
| use substreams::prelude::BigInt; | |||
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.
Not necessarily in this PR but if we start using this in many protocols I wonder if we shouldn't move it to tycho-substreams to avoid code repetition
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.
Literally had the same comment 😆
louise-poole
left a comment
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.
Nice! I'm very happy with the progress so far! Left a few comments, but overall the logic is sound and best practices were applied. Nice going 👍
substreams/ethereum-rocketpool/src/modules/1_map_protocol_components.rs
Outdated
Show resolved
Hide resolved
substreams/ethereum-rocketpool/src/modules/1_map_protocol_components.rs
Outdated
Show resolved
Hide resolved
| /// Returns: | ||
| /// `Vec<Attribute>`: A vector containing Attributes for each change detected in the tracked | ||
| /// slots. Returns an empty vector if no changes are detected. | ||
| pub fn get_changed_attributes( |
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 feel like it's warranted to move this into the tycho-substreams crate... it's used by a few protocols now. We should do that on a separate PR and merge it before updating this PR to use it.
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 we're discussing changing how we track liquidity, however these are the corrections that would be needed for the current implementation if we were to stick with using events and deltas. The clarification of using "total_eth" as the component balance applies either way though.
substreams/ethereum-rocketpool/src/modules/2_map_relative_component_balance.rs
Outdated
Show resolved
Hide resolved
substreams/ethereum-rocketpool/src/modules/4_map_protocol_changes.rs
Outdated
Show resolved
Hide resolved
substreams/ethereum-rocketpool/src/modules/4_map_protocol_changes.rs
Outdated
Show resolved
Hide resolved
|
Thank you for the reviews, will address it later today |
e8b009b to
6559898
Compare
louise-poole
left a comment
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.
Looks great! I want to review the related simulation PR before I approve here (just to confirm we covered all necessary attributes etc). Left a few smaller comments in the meantime.
substreams/ethereum-rocketpool/src/modules/4_map_protocol_changes.rs
Outdated
Show resolved
Hide resolved
substreams/ethereum-rocketpool/src/modules/4_map_protocol_changes.rs
Outdated
Show resolved
Hide resolved
substreams/ethereum-rocketpool/src/modules/4_map_protocol_changes.rs
Outdated
Show resolved
Hide resolved
substreams/ethereum-rocketpool/src/modules/2_map_protocol_changes.rs
Outdated
Show resolved
Hide resolved
d01ed6e to
78ed6c8
Compare
louise-poole
left a comment
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.
Looks great! Just some minor suggestions. Will approve once I see simulation work properly (I think we're close!).
substreams/ethereum-rocketpool/src/modules/2_map_protocol_changes.rs
Outdated
Show resolved
Hide resolved
substreams/ethereum-rocketpool/src/modules/2_map_protocol_changes.rs
Outdated
Show resolved
Hide resolved
substreams/ethereum-rocketpool/src/modules/2_map_protocol_changes.rs
Outdated
Show resolved
Hide resolved
f67accf to
64506e1
Compare
|
I have additionally updated the |
louise-poole
left a comment
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.
One last small adjustment and then I think we're ready! Once this is done, let's sync this in dev?
substreams/ethereum-rocketpool/src/modules/1_map_protocol_components.rs
Outdated
Show resolved
Hide resolved
louise-poole
left a comment
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.
Fantastic work on this 🔥
…nts and balance management (tests do not pass)
…plify error handling
… test configuration
…Capacity in simulation
…orage slots instead of deltas
…ames to snake case
… event handling post V1.2 deployment
…and asserting variable queue IDs
…s in protocol mapping
…act was activated and not from the point the V1.2 contract was deployed
…Rocket Pool Deposit Pool V1.2
…ot be running execution integration tests due to reasons explain in the integration_test
0848104 to
af223ff
Compare
No description provided.