[VEN-2985][VEN-3343] : VIP to add Flashloan Functionality in Core Pool . #609
Conversation
GitGuru7
left a comment
GitGuru7
left a comment
There was a problem hiding this comment.
we can now change the base to main, as emode branch is already merged.
chechu
left a comment
chechu
left a comment
There was a problem hiding this comment.
simulations/vip-555/utils/bsctestnet-cut-params.json LGTM
There was a problem hiding this comment.
I would move this file to the same folder of the first VIP. I don't think we'll enable the flash loan fees on mainnet, so we won't need this VIP on mainnet
There was a problem hiding this comment.
I think this would break other tests. Why don't we accept an optional parameter in the checkCorePoolComptroller function, so each test can provide a different account wallet (using ACCOUNT as a fallback)?
There was a problem hiding this comment.
testnet-addendum VIP, including its facet-cut-params, LGTM.
|
Testnet addendum execution: https://testnet.bscscan.com/tx/0x46016e8d58873c93864bc725b425fc878e20097d90c0e1d247fab072be7fcedc |
| export const NETWORK_ADDRESSES = { | ||
| bscmainnet: { | ||
| DEFAULT_PROPOSER_ADDRESS: "0x97a32D4506F6A35De68e0680859cDF41D077a9a9", | ||
| DEFAULT_PROPOSER_ADDRESS: "0x34221485302f6f2029660a000908b5fcabb9bc6e", |
There was a problem hiding this comment.
The previous proposer lacks sufficient voting power.
vips/vip-557/bscmainnet.ts
Outdated
| (timelock: string) => ({ | ||
| target: bscmainnet.ACCESS_CONTROL_MANAGER, | ||
| signature: "giveCallPermission(address,string,address)", | ||
| params: [UNITROLLER, "setFlashLoanPaused(bool)", timelock], |
There was a problem hiding this comment.
Should we also grant the guardian pause permission to ensure a quick response in case of any mishap? What do you think?
| }); | ||
|
|
||
| it("flash loans should be enabled for all core markets", async () => { | ||
| for (const market of CORE_MARKETS) { |
There was a problem hiding this comment.
Instead of hardcoding all market, I would prefer to get all the markets using this getAllMarkets().
There was a problem hiding this comment.
getAllMarkets() includes the non-upgradeable VBNB. We used the same pattern in the testnet VIP, so I prefer it for the mainnet as well.
| lens: NEW_COMPTROLLER_LENS, // NEW_COMPTROLLER_LENS | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
There should be some storage checks
| const marketContract = await ethers.getContractAt(VTOKEN_ABI, market.address); | ||
| expect(await marketContract.isFlashLoanEnabled()).to.equal(true); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Can we simulate a dummy flash loan transaction.
There was a problem hiding this comment.
I don't think it is required here.
| { | ||
| target: UNITROLLER, | ||
| signature: "diamondCut((address,uint8,bytes4[])[])", | ||
| params: [params], |
There was a problem hiding this comment.
anyway to generate this params ?
There was a problem hiding this comment.
Yes, we have a facet-cut-params-generator script in the Venus protocol.
| bscmainnet.NORMAL_TIMELOCK, | ||
| bscmainnet.FAST_TRACK_TIMELOCK, | ||
| bscmainnet.CRITICAL_TIMELOCK, | ||
| bscmainnet.GUARDIAN, |
There was a problem hiding this comment.
The bscmainnet.GUARDIAN is the Guardian 2 address itself:
Line 34 in 1171b17
vips/vip-557/bscmainnet.ts
Outdated
| params: [NEW_VBEP20_DELEGATE_IMPL, false, "0x"], | ||
| }; | ||
| }), | ||
| ...[bscmainnet.NORMAL_TIMELOCK, bscmainnet.FAST_TRACK_TIMELOCK, bscmainnet.CRITICAL_TIMELOCK].map( |
There was a problem hiding this comment.
let's add GUARDIAN2 as well
vips/vip-557/bscmainnet.ts
Outdated
| signature: "giveCallPermission(address,string,address)", | ||
| params: [UNITROLLER, "setFlashLoanPaused(bool)", timelock], | ||
| })), | ||
| ...[bscmainnet.NORMAL_TIMELOCK, bscmainnet.FAST_TRACK_TIMELOCK, bscmainnet.CRITICAL_TIMELOCK].map( |
vips/vip-566/bscmainnet.ts
Outdated
| const meta = { | ||
| version: "v2", | ||
| title: "VIP-557: Flash loan feature for all markets on BSC Mainnet", | ||
| description: "Upgrade contracts and enable flash loans on BSC Mainnet.", |
There was a problem hiding this comment.
curious, usually when we release a new feature, do we attach some doc here ? or maybe we should have detailed explanation ?
There was a problem hiding this comment.
Yes, for new features, we usually add a detailed description in the proposal metadata and link to relevant documentation.
|
rest lgtm |
This VIP updates the contracts to include the flashloan feature and enables them for the core markets on both testnet and mainnet with initial parameters.