-
Notifications
You must be signed in to change notification settings - Fork 29
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
Contango wand #531
base: tweak/update-utils
Are you sure you want to change the base?
Contango wand #531
Conversation
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 witch governance is missing, but's a really good start!
Updated yield-utils
8d3e078
to
f6e2b2b
Compare
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.
Only reviewed the code, not the tests yet.
|
||
function deployJoin(bytes6 assetId) external auth returns (IJoin join) { | ||
address asset = contangoCauldron.assets(assetId); | ||
require(asset != address(0), "Asset not known to the Contango Cauldron"); |
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.
require(yieldLadle.joins(assetId) == IJoin(address(0)), "Join already exists")
.
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.
good point
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.
btw, here I'm trying to avoid overriding an existing join, but I agree we should also check there isn't one that we could copy from the yieldLadle
witchDefaults = WitchDefaults(duration, vaultProportion, intialDiscount); | ||
} | ||
|
||
function setLineAndLimit( |
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.
What's the point of this function?
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.
have more granular access to the witch
} | ||
|
||
function setAuctioneerReward(uint256 auctioneerReward) external auth { | ||
contangoWitch.setAuctioneerReward(auctioneerReward); |
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.
There should be a boundAuctioneerReward
function, I guess.
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.
to set a min or max?
contangoWitch.setLineAndLimit(ilkId, baseId, duration, vaultProportion, collateralProportion, max); | ||
} | ||
|
||
function configureWitch(bytes6 ilkId, bytes6 baseId, uint128 max) external auth { |
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.
I guess this one should be named setLineAndLimit
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.
it was originally overloaded with that name, but that doesn't play well when you wanna get the selector to assign permissions
} | ||
|
||
/// @notice Propagate a token to the Ladle from the Yield Ladle | ||
function addToken(address token) external auth { |
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.
I like copyX
for when we are propagating configs from the yield ladle to the contango ladle, as you did below.
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.
yeah, most of the names were there and I didn't understand the pattern, so didn't change them
compositeOracle.setSource(baseId, ilkId, yieldSpaceOracle); | ||
} else if (yieldCauldron.assets(ilkId) != address(0)) { | ||
DataTypes.SpotOracle memory spotOracle_ = yieldCauldron.lookupSpotOracle(baseId, ilkId); | ||
compositeOracle.setSource(baseId, ilkId, spotOracle_.oracle); |
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.
Only if it is not IOracle(address(0))
, I think.
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 lookup method does that already
} | ||
|
||
/// @notice Add a new series, if it exists in the Yield Cauldron | ||
function addSeries(bytes6 seriesId) external auth returns (DataTypes.Series memory series_) { |
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.
As below, I think this should be copySeries
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.
ditto
AccessControl(address(yieldLadle.joins(USDT))).grantRole(root, address(wand)); | ||
AccessControl(address(yieldCauldron.series(FYUSDT2306).fyToken)).grantRole(root, address(wand)); | ||
|
||
wand.grantRole(wand.copySpotOracle.selector, address(this)); |
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 know that there is a grantRoles
function, right?
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.
yes, but creating the array is not as pretty as I'd like, and this way I can group the related permissions (wand + cauldron or whatever)
BTW, the PR seems to fail cause it requires to fork arbitrum, I have the feeling the secret with the URL is missing in the repo config (or misnamed) |
Still a draft. I've used copilot quite heavily, and there is a certain risk to this contract. We will need at least an internal audit.