-
Notifications
You must be signed in to change notification settings - Fork 7
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
Mainnet ready version #8
Conversation
…easier consumption
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.
Super exciting to see these contracts coming along! I didn't get a chance to fully review, but leaving some comments for what I did to kick off conversation.
Nothing major stood out, but I left a question about FeeCollector being a resource vs interface on BandOracleAdmin.
Will follow up to finish up the review tomorrow!
/// | ||
/// @return The `FeeCollector` resource | ||
/// | ||
pub fun createNewFeeCollector (): @FeeCollector { |
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 think if we're saying BandOracleAdmin
has authority to grant FeeCollector
privileges, they should have authority to revoke those privileges. As it stands, they can't do that if FeeCollector
is a resource defined in this contract, at least not without updating the contract and its logic around fee collection.
After looking at grant_fee_collector.cdc
, I think FeeCollector
should be an interface instead of being a standalone resource. That way granting FeeCollector
access privileges would be as simple as publishing a Capability on the BandOracleAdmin
to the desired fee collector account and the capability can be revoked if needed.
As I understand it, fees will be set to 0.0
until some point in the future which may not happen until Capability Controllers are released. But to be on the safe side, it would also make sense to have a path derivation function on the contract - something like:
access(all) fun deriveFeeCollectorPathIdentifier(forAddress: Address): String {
return BandOracle.FeeOraclePathPrefix.concat(forAddress.toString())
}
Then grant_fee_collector.cdc
would be a single-signed link & publish transaction, linking FeeCollector
Capability on the BandOracleAdmin
at the derived path and publishing to the recipient. The recipient then of course would claim the Capability and store it in their account.
And to revoke the FeeCollector Capability given to 0x05
I would just run
signer.unlink(BandOracle.deriveFeeCollectorPathIdentifier(forAddress: 0x05))
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.
Well idk about the authority to revoke those privileged, basically BandOracleAdmin
will be given to BandProtocol when they deploy the contract, latter they will co-sign a tx with Flow for give them the FeeCollector
. @franklywatson any thoughts on the "hierarchy" around 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.
Actually, we want to set a base fee from launch and so the contract will be used to set that as soon as it's deployed. Later when Band protocol starts charging a fee we'll drop ours and theirs will be passed on. Not sure I understand the point about Capability Controllers @sisyphusSmiling.
I think Gio's point is interesting but might be more than what is needed. The only FeeCollector will be some Flow account and there's really only one change I can think of that might apply here which is that Flow might want to fee collected by a different account, in which case another permission being granted is enough.
} | ||
|
||
/// | ||
/// The resource that will allow an account to make quote updates | ||
/// | ||
pub resource Relay { |
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.
nit: The name Relayer
makes sense to me here - consistent with FeeCollector and it's referred to in other parts of the contract as "relayer". I guess either works, but would prefer consistency across contract and naming conventions in their other contracts/docs
Co-authored-by: Giovanni Sanchez <108043524+sisyphusSmiling@users.noreply.github.com>
Co-authored-by: Giovanni Sanchez <108043524+sisyphusSmiling@users.noreply.github.com>
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.
Great work! 🚀
Closes: #3 #7
Description
fixedPointRate
field on theReferenceData
struct allowing consumers to use oracle data without casting it from the integer * e18 format.FeeCollector
feature that allows eventually to charge a fee for the use of the Oracle that will help maintaining it.For contributor use:
master
branchFiles changed
in the Github PR explorer