Closed stiiifff closed 3 months ago
@bekauz Open a draft PR so you can already take a look. /cc @uditvira
Some points left to be addressed:
Commit 6f8c91c962c35651139eb9d8eeb37a7805d8eea1 adds an initial_queue
instantiation param to the Clock which should forego the need of contracts to enqueue themselves into the Clock. (option 3 above)
Remaining to do:
looks great! just leaving a couple of (somewhat hypothetical) thoughts here, along with a few code comments.
given that in top level covenants we no longer add the ibc-forwarders to the whitelist, we could run into a situation where a forwarder is instantiated as part of a covenant, does something, and gets dequeued. now, because ibc forwarder was enqueued while bypassing the whitelist, we would not be able to re-queue it easily (e.g. firing
enqueue_clock
message) without update-migrating the clock's whitelist/initial queue state variables.some ideas that come to mind:
- continue adding the forwarders to the whitelist while also doing the
initial_queue
- automatically whitelist the initial queue contracts on clock instantiation
- skip the
initial_queue
altogether. "fire and forget"enqueue_clock
messages to every privileged address on ibc forwarder instantiationwhat do you think?
I think we could just leave the IBC forwarder in the whitelist, that would be the simplest thing to do. About the 3rd option, I was trying to avoid the dependency to the Clock contract .. isn't it required for the Enqueue msg ?
yeah, as it stands right now - clock dependency would be required for cases where privileged_addresses
would be non-empty (to send the enqueue message).
if having a dependency on the clock is a concern here, we could also move that enqueue
method to covenant-utils
which are used by all contracts. wdyt?
I guess that would work. But why should contracts enqueue themselves to another contract .. while the top-level covenant has all the info to act as orchestrator, and setup everything correctly (cfr. whitelist & initial_queue).
Some thoughts:
Will add a few more tests before merging, and create separate issues to migrate other contracts to use the same approach (privileged addresses), and to add an option to the top-level covenant to configure permissioned / permissionless ticking.
LGTM !
Allow the Tick operation to be either permissioned or permissionless based on a set of privileged addresses. Removed the dependency to the Valence clock .. still something to be addressed (see below in comments). Cleaned up IBC Forwarder tests related to testing the fallback address (to use a test address instead of reusing clock/next contract).
closes #217