Add Authorization-related events to the IApplication interface

[cygnusv]

Now that there are drafts for real application contracts for Threshold, I just realized we never standardized some application-specific stuff like events for authorization operations. As an example, see the parallel evolution of AuthorizationIncreased in the PRE and tBTC app contracts, respectively:

event AuthorizationIncreased(address indexed stakingProvider, uint96 fromAmount, uint96 toAmount);

(https://github.com/nucypher/nucypher/pull/2834/files#diff-a0f8e2c70e7a79b504a33807c59291816a3e3ed820aa1d195f79e500da6c861fR51)

vs.

event AuthorizationIncreased(address indexed stakingProvider, address indexed operator, uint96 fromAmount, uint96 toAmount);

(https://github.com/keep-network/keep-core/blob/main/solidity/ecdsa/contracts/WalletRegistry.sol#L184)

This can be problematic from interoperability and integrations point of view, particularly for the Threshold dashboard (cc @Battenfield ). As a solution, we need to find what's common on both implementations (considering also potential future application contracts), and add these events to the IApplication interface.

[pdyraga]

To be honest, I am happy they are so close to each other because we haven't looked at PRE repo when implementing this code 😆 tBTC (ECDSA) and Random Beacon has a concept of an operator address that is running the node and the operator is automatically updating sortition pool state, that's why we included indexed operator address in the event. I thought PRE also has a concept of an operator (worker)?

[cygnusv]

We should add also a minAuthorization() public view method to the application interface

[cygnusv]

tBTC (ECDSA) and Random Beacon has a concept of an operator address that is running the node and the operator is automatically updating sortition pool state, that's why we included indexed operator address in the event.

@pdyraga How can generalize this for all Threshold apps? I don't think it's fully established that the role of operator is general; even though both tBTC and PRE have an operator, it may be the case in the future that some new app has 0 or more than 1 operators. Also, from the point of view of what an authorization is and what it entails, the operator role doesn't seem relevant.

Since stakingProvider is already indexed, can you remove the indexed operator field from the event and use the self.stakingProviderToOperator and self.operatorToStakingProvider mappings that you have in EcdsaAuthorization? Another solution would be to define a second event, completely app-specific, that complements the AuthorizationIncreased.

[cygnusv]

Current divergences:

• AuthorizationIncreased:
  • Associated to function authorizationIncreased()
  • tBTC has an extra indexed operator field in the event (see discussion in previous comment)
• AuthorizationDecreaseRequested:
  • Associated to function authorizationDecreaseRequested()
  • tBTC has an extra uint64 decreasingAt field in the event.
    • @pdyraga Similarly to the comment above, this feels tBTC-specific. Is it possible to find a general semantics for this field? If not, I'd say let's remove it.
• AuthorizationDecreaseApproved:
  • Associated to a previous call to authorizationDecreaseRequested(), but the interface states the the request will be approved at the discretion of the application.
  • PRE has extra uint96 fromAmount and uint96 toAmount fields.
    • @pdyraga At the very least, the toAmount is required because synchronization issues may happen and this information can help. For reference, our staking contract has a method approveAuthorizationDecrease() called by applications in the same TX where AuthorizationDecreaseApproved is emitted on the application side.
• AuthorizationInvoluntaryDecreased:
  • tBTC has only an event for failures (InvoluntaryAuthorizationDecreaseFailed)
  • Instead, we should have a "positive" event for involuntary authorization decreases. PRE has event AuthorizationInvoluntaryDecreased(address indexed stakingProvider, uint96 fromAmount, uint96 toAmount)

Other suggested additions:

• function minAuthorization() returns (uint96) external view
• What about other minor metadata endpoints? e.g., name(), owner(), isUpgradeable()?

[pdyraga]

I agree adding minimumAuthorization function to IApplication makes sense. There is an implicit contract in TokenStaking that IApplication may revert in case the minimum authorization requirement is not met. The problem is that IApplication does not clearly define what is the minimum authorization. I added it in #91.

[pdyraga]

After thinking more about it, and even doing some experiments if the code, I think we should leave it as-is and don't care about what events are emitted by applications at all. Dashboard, subgraphs, and other tools should work with TokenStaking events instead.

We have these events in the staking contract: https://github.com/threshold-network/solidity-contracts/blob/47268908a1aa92ab215b76e5f26730519643b663/contracts/staking/TokenStaking.sol#L130-L154

If the staking contract says the authorization was increased for the application, it does not matter what the application says. The application may confirm by event that the internal state was indeed updated but I think we should not try to find a general semantic for that. Every application will handle it differently. One application may update the state immediately, and another may need to wait for some other conditions to switch (e.g. beacon waits for the sortition pool to be unlocked in case group creation is in progress).

If the staking contract says the authorization decrease was approved, we are sure this is true because it's the staking contract state that just got updated. For every correctly functioning application, application-specific approval event would be emitted in the same transaction as AuthorizationDecreaseApproved from the staking contract. I am not sure if it makes sense to duplicate them.

The same logic applies to AuthorizationDecreaseRequested and AuthorizationInvoluntaryDecreased.

All these events do not have the operator or any other application-specific fields and they all expose fromAmount and toAmount calculated based on staking contract info.

In this model, staking contract is the only source of truth. And I think it makes sense because we are sure that what is in the staking contract is right. Even if a malfunctioning application says something else in the events it is emitting. What do you think @cygnusv?

[vzotova]

Generally I agree with using TokenStaking as source of events. I see one reason to extract events to IApplication: if we will have some general dashbord for all apps or some system that will gather information from all apps. Anyway I think it's totally fine to not add them to general interface.

[pdyraga]

But for this use case we still can use TokenStaking, right? Each event has an application address indexed so all we need is a list of application addresses. This is also available in TokenStaking by scanning ApplicationStatusChanged events.

[vzotova]

True, but still possible desynchronization where events in apps can help.