spec: Health Triger Manager#12
Conversation
7f718bc to
f0745d8
Compare
f0745d8 to
e459fdc
Compare
2b48af5 to
3580e0e
Compare
|
|
||
| ### Registration | ||
|
|
||
| To register a health trigger the caller provides a position ID, a health function $$H(P)$$, and the three fields of a `HealthTrigger` (bounds, callback, provider). If the position already has an entry in the registry, the health function is ignored and the new trigger is appended to the existing entry's trigger list. |
There was a problem hiding this comment.
Seems dangerous, a malicious actor could register a position with a health function that always returns 1.5, essentially bricking all other callbacks on that position.
There was a problem hiding this comment.
I have a solution for this, basically requiring the provider to be a provider: Capability<auth(FungibleToken.Withdraw) &FlowToken.Vault> do you still see a way it could panic?
There was a problem hiding this comment.
never mind actually. this could still panic! I will change this altogether
| **Validation:** The **HTM** evaluates $$H(P)$$ at registration time. Registration is rejected if: | ||
|
|
||
| - $$H(P)$$ returns `nil` (the position does not exist or its health cannot be determined). | ||
| - The position is already out of bounds ($$H(P) < H_{\textbf{min}}$$ or $$H(P) > H_{\textbf{max}}$$, for whichever bounds are specified). This prevents registering triggers that would fire immediately, which would be wasteful and could mask bugs in the caller's logic. |
There was a problem hiding this comment.
If possible the registration function should not panic, or we would have to wrap any registration from the FYV into a scheduled transaction.
If the health function never panics this is fine.
However in almost all cases this should have been already checked by the calling function and if health has not been checked by the calling function, then it would have to check if after registration.
Is there any reason we shouldn't accept registrations which trigger immediatly?
There was a problem hiding this comment.
I can make registration more lax. I just saw no point registering something that will trigger immediately, but maybe it makes the rest of the code easier.
I will see if we can add a guarantee to the Register function that it will not panic.
There was a problem hiding this comment.
made registration more lax and added guarantee it wont panic, but I had to add a limitation that you cannot have high priority scheduling. c0f5578
| - If $$H_{\textbf{min}} \leq H(P) \leq H_{\textbf{max}}$$ (within bounds) → no action. Continue to the next trigger. | ||
| - The trigger is out of bounds. Validate capabilities: | ||
| - The `HealthCallback` capability still exists. | ||
| - The `Provider` capability still exists and has sufficient funds to pay for the callback TX. |
There was a problem hiding this comment.
Provider is an interface and may panic, disrupting everything indefinitely.
There was a problem hiding this comment.
made provider more specific so that it has to have a flow vault underneath.
|
|
||
| **Return value:** Registration returns success or failure. Failure indicates one of the validation checks above was not met. | ||
|
|
||
| **Access control:** Registration is public. Since the registering party pays for callback execution (via the `Provider` capability), there is no execution cost to the protocol and no need for additional permissioning. Storage costs are absorbed by the **HTM** account for now (see [Future Expansions](#7-future-expansions)). |
There was a problem hiding this comment.
I like to assume that its public for any of the other security considerations to protect against our own misuse.
But I would restrict it at the beginning.
There was a problem hiding this comment.
Looking at this again, registration might need to be guarded, but process doesn't need to be... I will check.
There was a problem hiding this comment.
registration is now gated and does not accept a provider: 30cce52
3580e0e to
5b20a9b
Compare
|
|
||
| The registry is keyed by position ID ($$P$$ = `position.id`, i.e. the Position resource's UUID). Each entry contains a list of `HealthTrigger` objects for that position. | ||
|
|
||
| Position health is obtained via `FlowALP.Pool.getPositionHealth(positionID)`, which returns `UFix64?`. A `nil` return means the health could not be determined (see [Process](#process) for how this case is handled). The `Registry` holds a single `Capability<&FlowALP.Pool>`, set at init and changeable by admin. This ensures [Process](#process) calls the health function only once per position, regardless of how many triggers are registered for it. |
There was a problem hiding this comment.
2 thoughts, expanding the Q&A session today:
- I think for the immediate term it's fine to just say
getPositionHealthwill panic if we can't get the health for any reason. We don't expect that to happen, but if it does we'll manually restart the HTM. - When we need to deal with recoverable error conditions, I agree with Alex that we should explicitly encode those in the return value of the fail-able function.
There was a problem hiding this comment.
So you are suggesting to just panic in case of nil for now and we can later add the disambiguation?
If that is the case, wouldn't it be simpler/cleaner to just have health return an UFix instead of UFix? for now?
There was a problem hiding this comment.
I'm just saying we have the freedom to panic, now, in the implementation, if that simplifies your life.
The spec should be more forward-looking. I think the spec should, when describing how we deal with multiple distinct failure conditions, use Alex's suggestion of enumerating the possible failure modes in the return type. Since we haven't decided on a structure for doing that, I would just say:
getPositionHealthreturns either a health value, or an error.- The error structure is TBD, and should be a common pattern across the project, but would be similar to [
Eithertypes](https://hackage-content.haskell.org/package/base-4.22.0.0/docs/Data-Either.html - Before this error handling structure is defined, implementations may panic on failure. (if this saves time, or we can just define and implement the structure)
| hMin: UFix64?, | ||
| hMax: UFix64?, | ||
| callbackAddress: Address, | ||
| reason: String |
There was a problem hiding this comment.
| reason: String | |
| reason: TriggerRemovalReason |
Maybe define an enum type for reason? Although I guess Cadence probably just puts the raw enum integer value into the event data -- it would be nice to still have the string representation in the actual event.
There was a problem hiding this comment.
Not sure about this one! There is noone on chain that will be looking for this info and the offchain processes wont care that this is an enum. Enums also have issues with contract upgrades... so if not really necessary I would say lets just have strings.
No description provided.