Skip to content

Review-automation#311

Merged
mkflow27 merged 51 commits into
mainfrom
review-automation
Apr 16, 2025
Merged

Review-automation#311
mkflow27 merged 51 commits into
mainfrom
review-automation

Conversation

@mkflow27
Copy link
Copy Markdown
Contributor

@mkflow27 mkflow27 commented Mar 20, 2025

This adds review automation for rate providers with a new template and review scope. The scope:

  • Check on proxy upgradeability for rate providers and downstream contracts
  • Check interface implementation accuracy ( hasValidGetRateFunction )
  • Checks rate is scale18 ( isRateScale18 )

Will close #322

Copy link
Copy Markdown
Contributor

@brunoguerios brunoguerios left a comment

Choose a reason for hiding this comment

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

It's awesome to see this being built 👏
Seems like we're almost there 😃
Other than the comments I added, if you could add an example comparing it with an existing review, it would be nice to compare results.

Comment thread src/app.ts
*/
public async getIsRateProviderUpgradeable(): Promise<boolean> {
const upgradeableContracts = await this.getUpgradeableContracts()
return upgradeableContracts.some((c) => c.wasUpgraded)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this indeed a good way to detect if it's upgradeable? I mean, is it possible that it's upgradeable, but it wasn't upgraded?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This needs to be reworked.

Comment thread src/app.ts
}

// rate must be scale 18
const isScale18 = data < 9n * 10n ** 18n && data >= 1n * 10n ** 18n ? true : false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't follow this check 🤔
Are you checking if rate is between 1e18 and 9e18?
I've seen rates of 1000e18, so that check would fail.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Curious if you recall the types of pools where this is the case. I am aware of the following two scenarios:

  • "oracle pool" Where the pools tokens are not alike. Like our USDC-WETH pool on Base with the stableSurge
  • Gyros ConstantRateProviders

All other rate providers I recall would fall within this testing scope I think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Apparently I mixed convertToAssets and getRate results 😅
I was thinking about scenarios where wrapped and underlying token decimals were different (e.g. csUSDC).
The ones I checked do fall within the scope you created, so we should be fine 👍
Even though I don't feel like it's a super safe way to check for scale18, I couldn't come up with a better way 😬
I'd suggest wee keep an eye on rate providers that fail at this criteria so we can make a sanity check before confirming it doesn't properly implement this function.

@MattPereira
Copy link
Copy Markdown
Contributor

@mkflow27 you might consider adding some noob friendly getting started instructions to the README about how to install, run the script, ect.

@MattPereira
Copy link
Copy Markdown
Contributor

MattPereira commented Mar 21, 2025

I'm struggling to get the script to run

npm run write-review --rateProviderAddress 0xdDDF909076B641C51f22ACD4b134C54adad51e68 --network mainnet --rateProviderAsset 0x657e8C867D8B37dCC18fA4Caead9C45EB088C642

results in

SyntaxError: Unexpected non-whitespace character after JSON at position 4 (line 1 column 5)

I pulled the tenderly account / project slugs and generated API key from

https://dashboard.tenderly.co/balancer/sdk-team/settings

It looks like the response in getTenderlySimulation() is failing with not found

Response {
  status: 404,
  statusText: 'Not Found',
  headers: Headers {
    'content-type': 'text/plain',
    'user-agent': 'node',
    vary: 'Origin,Origin',
    date: 'Fri, 21 Mar 2025 19:04:29 GMT',
    'content-length': '18',
    'x-envoy-upstream-service-time': '0',
    'x-ratelimit-limit': '600, 600;w=60',
    'x-ratelimit-remaining': '599',
    'x-ratelimit-reset': '31',
    server: 'envoy',
    via: '1.1 google',
    'alt-svc': 'h3=":443"; ma=2592000,h3-29=":443"; ma=2592000'
  },
  body: ReadableStream { locked: false, state: 'readable', supportsBYOB: true },
  bodyUsed: false,
  ok: false,
  redirected: false,
  type: 'basic',
  url: 'https://api.tenderly.co/api/v1/account/balancer/sdk-team/project/simulate'
}

Copy link
Copy Markdown
Contributor Author

@mkflow27 mkflow27 left a comment

Choose a reason for hiding this comment

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

I'm struggling to get the script to run

npm run write-review --rateProviderAddress 0xdDDF909076B641C51f22ACD4b134C54adad51e68 --network mainnet --rateProviderAsset 0x657e8C867D8B37dCC18fA4Caead9C45EB088C642

results in

SyntaxError: Unexpected non-whitespace character after JSON at position 4 (line 1 column 5)

I pulled the tenderly account / project slugs and generated API key from

https://dashboard.tenderly.co/balancer/sdk-team/settings

It looks like the response in getTenderlySimulation() is failing with not found

Response {
  status: 404,
  statusText: 'Not Found',
  headers: Headers {
    'content-type': 'text/plain',
    'user-agent': 'node',
    vary: 'Origin,Origin',
    date: 'Fri, 21 Mar 2025 19:04:29 GMT',
    'content-length': '18',
    'x-envoy-upstream-service-time': '0',
    'x-ratelimit-limit': '600, 600;w=60',
    'x-ratelimit-remaining': '599',
    'x-ratelimit-reset': '31',
    server: 'envoy',
    via: '1.1 google',
    'alt-svc': 'h3=":443"; ma=2592000,h3-29=":443"; ma=2592000'
  },
  body: ReadableStream { locked: false, state: 'readable', supportsBYOB: true },
  bodyUsed: false,
  ok: false,
  redirected: false,
  type: 'basic',
  url: 'https://api.tenderly.co/api/v1/account/balancer/sdk-team/project/simulate'
}

script runs fine for me with the properties you shared. I ran npm run write-review -- --rateProviderAddress 0xdDDF909076B641C51f22ACD4b134C54adad51e68 --network mainnet --rateProviderAsset 0x657e8C867D8B37dCC18fA4Caead9C45EB088C642 (added two dashes)

However I am also using my own account which is not related to the balancer org. Let me ask to get access there as well. The resulting tenderly sim url is: https://dashboard.tenderly.co/shared/simulation/7aa2096c-cbe8-4304-b987-d78f096cbdc2

@mkflow27 mkflow27 requested a review from franzns March 28, 2025 13:29
@mkflow27 mkflow27 linked an issue Apr 3, 2025 that may be closed by this pull request
@mkflow27 mkflow27 merged commit cc4a4af into main Apr 16, 2025
1 check passed
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.

Monitoring agents Automatic rate provider reviews

3 participants