wormhole
wormhole copied to clipboard
Implementation of 'Flow Cancel' modifications to Governor calculations
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
.
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 theemitter
- A transfer's
Timestamp
is within the usual 24 hour window used by the Governor - The asset in the transfer has fields
OriginChain
andOriginAddress
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
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 Transfer
s 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.
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.
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 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.
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.