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

Balancer Strategy Code Simplification #1851

Merged

Conversation

shahthepro
Copy link
Collaborator

If you made a contract change, make sure to complete the checklist below before merging it in master.

Refer to our documentation for more details about contract security best practices.

Contract change checklist:

  • Code reviewed by 2 reviewers.
  • Copy & paste code review security checklist below this checklist.
  • Unit tests pass
  • Slither tests pass with no warning
  • Echidna tests pass if PR includes changes to OUSD contract (not automated, run manually on local)

@rafaelugolini rafaelugolini temporarily deployed to preview-oeth-shah-balan-9tyy5j October 4, 2023 11:22 Inactive
@rafaelugolini rafaelugolini temporarily deployed to preview-ousd-shah-balan-9h5re4 October 4, 2023 11:22 Inactive
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Warnings
⚠️ 👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against 5ac1ac1

@shahthepro shahthepro temporarily deployed to preview-ousd-shah-balan-9h5re4 October 4, 2023 11:37 Inactive
@shahthepro shahthepro temporarily deployed to preview-oeth-shah-balan-9tyy5j October 4, 2023 11:37 Inactive
@shahthepro shahthepro temporarily deployed to preview-oeth-shah-balan-9tyy5j October 4, 2023 11:45 Inactive
@shahthepro shahthepro temporarily deployed to preview-ousd-shah-balan-9h5re4 October 4, 2023 11:45 Inactive
@shahthepro shahthepro temporarily deployed to preview-oeth-shah-balan-9tyy5j October 4, 2023 11:47 Inactive
@shahthepro shahthepro temporarily deployed to preview-ousd-shah-balan-9h5re4 October 4, 2023 11:47 Inactive
@shahthepro shahthepro temporarily deployed to preview-oeth-shah-balan-9tyy5j October 4, 2023 12:14 Inactive
@shahthepro shahthepro temporarily deployed to preview-ousd-shah-balan-9h5re4 October 4, 2023 12:14 Inactive
@shahthepro shahthepro temporarily deployed to preview-oeth-shah-balan-9tyy5j October 4, 2023 12:48 Inactive
@shahthepro shahthepro temporarily deployed to preview-ousd-shah-balan-9h5re4 October 4, 2023 12:48 Inactive
@shahthepro shahthepro temporarily deployed to preview-ousd-shah-balan-9h5re4 October 4, 2023 12:52 Inactive
@shahthepro shahthepro temporarily deployed to preview-oeth-shah-balan-9tyy5j October 4, 2023 12:52 Inactive
@shahthepro shahthepro marked this pull request as ready for review October 4, 2023 12:53
@shahthepro shahthepro temporarily deployed to preview-oeth-shah-balan-9tyy5j October 4, 2023 12:57 Inactive
@shahthepro shahthepro temporarily deployed to preview-ousd-shah-balan-9h5re4 October 4, 2023 12:57 Inactive
Copy link
Member

@sparrowDom sparrowDom left a comment

Choose a reason for hiding this comment

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

Awesome stuff thank you for this

@@ -537,22 +536,30 @@ abstract contract BaseBalancerStrategy is InitializableAbstractStrategy {
virtual
returns (uint256)
{
IRateProvider rateProvider = poolRateProvidersCache[
Copy link
Member

Choose a reason for hiding this comment

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

this function is really nicely simplified!

/**
* @notice Caches Pool Assets and their index. These will never change
* in a strategy. It's `public` because we already have one strategy
* initialized without this, would make it easier when upgrading it
Copy link
Member

Choose a reason for hiding this comment

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

thanks for thinking this through 🙏

@sparrowDom sparrowDom merged commit be15ba6 into sparrowDom/balancer-composable-st-pool Oct 4, 2023
@sparrowDom sparrowDom deleted the shah/balancer-gas-opts branch October 4, 2023 14:58
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.

3 participants