Implementation of 'Flow Cancel' modifications to Governor calculations

[johnsaigle]

EDIT

The flow cancel logic has been rewritten following Bruce's feedback. Updated design can be found in the commit message here: 21c96c1

Overview

This pull requests contains modifications to the Governor.
In particular, it allows a certain allow-list of incoming tokens to "pay down" the Governor usage for the destination chain.

Note: This implementation is meant to be minimally invasive to highlight the flow cancel design and mechanism. As a result, it is not very efficient. Once the overall design is clear and agreed upon, I am more than happy to refactor the code to make it more performant.

Background

The motivation and design document for this change is detailed in the GitHub Discussion "Wormhole Governor - Flow-Canceling Design Proposal (Temp Check)".

Note that this feature depends on and includes commits from Bruce's pull request, namely the modifications to node/pkg/db/governor.go. (The other PR was merged and this one is rebased to include it)

Details

Allow list

The allow-list is in the same format as other hard-coded token implementations in Wormhole, such as the 'manual token list'.

Currently, the allow list contains all implementations of UDSC, USDT, and DAI that are present in the 'generated mainnet token list'

Flow Cancel

A 'flow cancel' transfer occurs when all of the following are true:

• An emitter chain is governed by the Governor
• A transfer has a TargetChain equal to the emitter
• A transfer's Timestamp is within the usual 24 hour window used by the Governor
• The asset in the transfer has fields OriginChain and OriginAddress equal to an entry in the allow list

Example scenario

• A particular USDC implementation minted on Ethereum. It is allow-listed.
• A transfer of $1,000 of this asset is sent from Solana to Sui.
• Before the transfer, Sui's usage is $10,000.
• Before the transfer, Solana's usage is $0.

After the transfer is processed, Sui's new usage will be $9,000 (flow cancel) and Solana's new usage will be $1,000 (as usual).

Areas of interest for reviewers

Efficiency (resolved by rewrite)

There is a triple for-loop involved in the calculations. (For every governed chain --> for every transfer --> for every allow-listed asset ...)

Obviously this isn't optimal. However, I wanted to implement the logic using the existing database structure and Go structs where possible so that this change is not too intrusive.

In general, anything we can do to optimize the way Transfers are filtered and accessed would be a big improvement. While there are 3 loops involved here, the token list and gov.chains are small, fixed numbers. Our real issue is processing the transfers.

One clear improvement would be to make a struct similar to chainEntry that indexes based on target chain, rather than emitter chain. This would allow for more efficient lookups when calculating the net flows into a chain via the FlowCancellingTransfersForChain().

Another possible approach is to cache the Governor usage calculations, perhaps in the ChainGovernor.

Some limitations:

• The sum values in governor.go as well as the Value of transfers are unsigned ints.
• The governor usage is dynamically calculated and not stored in the database
• chainEntry's are indexed by source (emitter) chains, not destination (target).

Another area for improvement: FlowCancelTokenList() is a function, not a field of ChainGovernor but it probably could be (similarly to its fields tokens and tokensByCoinGeckoId).

Contents of the allow-list

Are there any assets that should be removed? I wanted to include all stablecoins from the generated mainnet tokens list and to avoid making editiorial decisions about specific tokens. Felix suggested in a review to use only the assets originating from Ethereum and Solana chains for an initial rollout.

That said, there are some likely candidates for removal; the following appear to have de-pegged:

• https://www.coingecko.com/en/coins/bridged-tether-orbit-bridge
  {chain: 13, addr: "000000000000000000000000cee8faf64bb97a73bb51e115aa89c17ffa8dd167", symbol: "oUSDT", coinGeckoId: "orbit-bridge-klaytn-usd-tether", decimals: 6, price: 0.490247}, // Addr: 0xcee8faf64bb97a73bb51e115aa89c17ffa8dd167, Notional: 1.5499198124759999Z}

• https://www.coingecko.com/en/coins/klaytn-dai
  {chain: 13, addr: "0000000000000000000000005c74070fdea071359b86082bd9f9b3deaafbe32b", symbol: "KDAI", coinGeckoId: "klaytn-dai", decimals: 18, price: 0.490008}, // Addr: 0x5c74070fdea071359b86082bd9f9b3deaafbe32b, Notional: 0.00980016

Test coverage

Unit tests

I added unit tests to cover the new functionality. I'm happy to add more if others have suggestions.

Dynamic test coverage

I got as far as running Tilt on my machine but I'm not exactly clear on the best way to run smoke tests for this feature.

Benchmarking

As we refine these changes, it would be beneficial to benchmark the Flow Cancelling calculations with a large number of transfers as it is likely to introduce a performance hit.

[bruce-riley]

Couldn't this be done more easily / efficiently by adding the cancelling transfers into emitterChainEntry.transfers for the target chain with a negative value? Then the existing summation should just work.

This would mean making the payload of the transfers array have the value as a separate data item so it could be negative. You would also have to change reloadTransfer to apply the cancellations to the target chain.

Could add a flowCancel flag to tokenEntry and set that for everything in FlowCancelTokenList. Then check that in ProcessMsgForTime and add the negative value to the target chain if the flag is set.

Happy to discuss this offline if you like.

[bruce-riley]

Please see my comment above.

[johnsaigle]

@bruce-riley I don't have objections to that method in general, and I looked into it following our intial discussion. The main caveat that most of the values in question (in the structs and database IIRC) are using unsigned types, so it's not trivial to modify the code to use negative values. This would also limit the effective upper bound of the values (since half of the data type would need to be reserved to represent negative values.)

It may be possible to change to e.g. int64 instead of uint64 but depending on how these values are used across the codebase, it may be that pulling on the thread actually ends up being a much more invasive change.

[bruce-riley]

The value is in USD, so I don't think we need to worry about exceeding the max of an int64. Also, I'm not proposing changing the DB. That would need to stay what's in the actual transfer. The signed value could be stored in the transfers array in addition to the DB Transfer object (so the new payload in that array would be a struct containing the DB transfer and a signed value).

Anyway, I think it would be useful to have another face to face discussion to talk through the alternatives.

[SEJeff]

Lots of nits, mostly style stuff since I know you're not a gopher natively, but this is good. I couldn't find anything obviously bad and this is definitely a big improvement.