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-2846]: Add SolvBTC market #407

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

[VEN-2846]: Add SolvBTC market #407

wants to merge 6 commits into from

Conversation

kkirka
Copy link
Contributor

@kkirka kkirka commented Oct 7, 2024

This PR adds SolvBTC market to Venus Core (Legacy) pool on BNB mainnet. This PR also contains scripts in the framework to check risk parameters in legacy pool.

@@ -26,6 +26,7 @@ export const ORACLE_BNB = "0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB";
export const NETWORK_ADDRESSES = {
bscmainnet: {
DEFAULT_PROPOSER_ADDRESS: "0x97a32D4506F6A35De68e0680859cDF41D077a9a9",
ACCESS_CONTROL_MANAGER: "0x4788629ABc6cFCA10F9f969efdEAa1cF70c23555",
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we use the address from the npm package in this case (as we are doing for bsctestnet)? Anyway, I'm not sure if should add to this file the ACM address, because the framework doesn't use it. We could simply define this address in the VIP's where we need it. I think that is the general rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why don't we use the address from the npm package in this case

Address casing is incorrect in the governance repo (it's all lowercase there), I'll make a PR to change that

I'm not sure if should add to this file the ACM address, because the framework doesn't use it

I think the framework shoud use it, it's one of our main contracts and its address is unlikely to change in future. The direction we seem to be going with the framework is to offload more responsibilities to it (e.g. it already checks risk params and market configuration). It could also check ownership / access controls at some point. I can add the corresponding checks (i.e. move

it("has correct owner", async () => {
expect(await vToken.admin()).to.equal(NORMAL_TIMELOCK);
});
it("has correct ACM", async () => {
expect(await vToken.accessControlManager()).to.equal(ACCESS_CONTROL_MANAGER);
});
to the framework).

I think that is the general rule

Unrelated to all of the above, I think we should discuss this further. It's true that at some point we decided to use hardcoded addresses in VIPs. However, IMO, with more and more networks and deployments the PR reviews become quite time consuming and error-prone – we need to manually check every address, and the more addresses we have hardcoded, the easier it is to miss a mistake in one of them. I think there should be a better approach here (maybe something along the lines of what I mentioned in this thread). If we could check the deployment commit hash rather than all the addresses during the reviews, this would simplify things and make errors less probable.

vips/vip-400/bscmainnet.ts Show resolved Hide resolved
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