-
Notifications
You must be signed in to change notification settings - Fork 62
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
Feature/cover v2 #128
Feature/cover v2 #128
Conversation
contracts/interfaces/ICover.sol
Outdated
} | ||
|
||
struct Product { | ||
uint16 productType; | ||
address productAddress; | ||
uint16 capacityFactor; | ||
uint24 productId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need a productId here. The id is the index itself. Also bring back productAddress since there is plenty of space in this slot and there's no need to make another read from a different slot when redeeming incident payouts.
uint maxPrice, | ||
StakingPool[] calldata stakingPools | ||
uint maxPremiumInAsset, | ||
CoverChunkRequest[] calldata coverChunkRequests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Cover prefix is a bit redundant given that we're in the context of cover anyway. Is using a struct for calldata helpful? I'd rather see it as two arrays (pool ids and amounts) since it's only used once here.
|
||
Cover[] public override covers; | ||
mapping(uint => StakingPool[]) stakingPoolsForCover; | ||
CoverData[] public override covers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just Cover?
)); | ||
} | ||
|
||
function addProduct( | ||
uint16 productType, | ||
address productAddress, | ||
uint24 productId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed here, but the productAddress is.
|
||
/* === MUTATIVE FUNCTIONS ==== */ | ||
|
||
function buyCover( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
contracts/modules/cover/Cover.sol
Outdated
{ | ||
IPool pool = pool(); | ||
// convert to NXM amount | ||
tokenPrice = pool.getTokenPrice(payoutAsset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pool.getTokenPrice -> pool().getTokenPrice
This means you can remove the lone above.
contracts/modules/cover/CoverNFT.sol
Outdated
|
||
contract CoverNFT is ERC721 { | ||
|
||
ICover cover; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cover
is a state variable and no visibility is specified. There's no way to modify it so there's no point in having it in storage - let's make it immutable (cover is a proxy anyway) and let's make it public.
contracts/modules/cover/Cover.sol
Outdated
uint public capacityFactor; | ||
uint public coverCount; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make capacityFactor
and coverCount
uint32 and move them near coverNFT
address so they can use the same slot (32+32+160 = 224).
uint24 productId; | ||
uint8 payoutAsset; | ||
uint96 amount; | ||
uint32 start; | ||
uint32 period; // seconds | ||
uint96 price; | ||
uint96 premium; | ||
} | ||
|
||
struct Product { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pack initialPrice
and activeCoverAmountInNXM
in here as well.
No description provided.