Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Beefing up the Coordinator: Restrictions to initiator, ritual duration, fee model, automatic TX reimbursements, etc #86
Beefing up the Coordinator: Restrictions to initiator, ritual duration, fee model, automatic TX reimbursements, etc #86
Changes from all commits
41f1634
e56c3ef
f24fd29
deb7fcf
b8492bf
7ccae5b
66e1f64
64792c1
d13d287
ddb0847
f11e8c4
486b893
cf9e74a
55fa421
a896ed0
85cc9c2
d634394
02b93ed
f819fac
bc11d12
362891e
b2eb840
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Shall we permit the
feeModel
address to be updated by the proper role?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, it's possible, but I'd rather handle that in a later PR, when we decide how do we want to manage fee models (see comment in L71). For our immediate needs (devnets), I don't foresee we will want to change the fee model
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.
Could be
authority
zero address?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.
StartRitual
's 2nd parameter is indexed as "initiator
":ritual.initiator
?initiator
andauthority
be in the event?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 catch. I meant
authority
here. IMO, once we introduce the authority role, the initiator is nothing more than a sponsor so there's in principle not much interest on it. In fact, once the fee is fully processed (so there's no refund), this role is not needed anymore. I would lean towards just including the authority in the event.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.
Since the 'paywall' here appears to be generating a public key for the first time, I'm wondering how a sponsor tops-up the availability horizon without a new ritual, and also if once they are conferred the
INITIATOR_ROLE
, they can do this unilaterally and whenever they wantThere 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 questions. Let's track these here #93
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 proposal contains a default
duration
fixed at 182.5 days for initializationand then top ups are fixed at 30 days (payable every 30 days) to keep the availability horizon 182.5 days into the future
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.
Is this something you would expect to be hardcoded at the smart contract level?
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'm opening an issue to track top-ups #93
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.
No not hardcoded, as ofc we may need to change these. Just wanted to note the likely genesis defaults, pending feedback. However there is a small question of trust implications post-v7.0.0 – ideally the Threshold DAO would control sponsorship parameters like these