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

[VEN-3074] Add vip for the wstETH pool for Base and Zksync #506

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

Conversation

Debugger022
Copy link
Contributor

@Debugger022 Debugger022 commented Feb 19, 2025

Description

  • For ZKSYNC sepolia use below command
    anvil-zksync fork --fork-url sepolia-testnet --fork-block-number 4761402

Resolves VEN

@Debugger022 Debugger022 self-assigned this Feb 19, 2025
@Debugger022 Debugger022 requested a review from chechu February 21, 2025 08:28
@Debugger022 Debugger022 marked this pull request as ready for review February 21, 2025 14:40
@Debugger022 Debugger022 requested a review from kkirka February 24, 2025 08:36
@Debugger022 Debugger022 requested a review from GitGuru7 February 24, 2025 08:57
@Debugger022 Debugger022 requested a review from chechu February 24, 2025 13:00
Copy link
Member

@chechu chechu left a comment

Choose a reason for hiding this comment

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

Commands (mainnet and testnet) lgtm

Comment on lines 50 to 54
describe("vTokens deployment", () => {
it(`should deploy market`, async () => {
await checkVToken(zksyncMarket.vToken.address, zksyncMarket.vToken);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe("vTokens deployment", () => {
it(`should deploy market`, async () => {
await checkVToken(zksyncMarket.vToken.address, zksyncMarket.vToken);
});
});
describe("vTokens deployment", () => {
checkVToken(zksyncMarket.vToken.address, zksyncMarket.vToken);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

);
});

describe("generic tests", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe("generic tests", async () => {
describe("generic tests", () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +143 to +145
checkIsolatedPoolsComptrollers({
[COMPTROLLER_CORE_ZKSYNC]: WSTETH_HOLDER,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't actually check supplying or borrowing wstETH. Rather, with the current implementation of checkIsolatedPoolsComptrollers it would skip the checks (note the absence of "supplying X, borrowing Y" tests):

image

Might be ok to leave it as is for this VIP, though, it's seemingly out of the scope.

Copy link
Contributor

@kkirka kkirka left a comment

Choose a reason for hiding this comment

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

zksyncmainnet and basemainnet risk params, commands and addresses LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants