Zero-amount transaction fee (GAIA) transactions are not observed by THORChain
Example transactions:
https://www.mintscan.io/cosmos/transactions/A8790E58AD0D88B26B57024C678D32297E7162A2B9C5369C0C6E938E2A757B0F
https://www.mintscan.io/cosmos/transactions/ECE8B5E9C581C19DE76E82780B1EC559B2175AB4282261E32F950AA3D8FB6297
Discord threads:
https://discord.com/channels/838986635756044328/1136662820587589684/1136746211878117448
https://discord.com/channels/838986635756044328/1149147041549783071/1149185741260337172
Code comments from the first Discord thread:
[quote]
MsgObservedTxIn ValidateBasic
:
https://gitlab.com/thorchain/thornode/-/blob/v1.116.0/x/thorchain/types/msg_observed_txin.go#L30-31
if err := tx.Valid(); err != nil {
return cosmos.ErrUnknownRequest(err.Error())
ObservedTx Valid
:
https://gitlab.com/thorchain/thornode/-/blob/v1.116.0/x/thorchain/types/type_observed_tx.go#L31-32
if err := m.Tx.Valid(); err != nil {
return err
Tx Valid
:
https://gitlab.com/thorchain/thornode/-/blob/v1.116.0/common/tx.go#L207-208
if err := tx.Gas.Valid(); err != nil {
return err
Gas Valid
:
https://gitlab.com/thorchain/thornode/-/blob/v1.116.0/common/gas.go#L69-71
for _, coin := range g {
if err := coin.Valid(); err != nil {
return err
Coin Valid
:
https://gitlab.com/thorchain/thornode/-/blob/v1.116.0/common/coin.go#L89-90
if c.Amount.IsZero() {
return errors.New("amount cannot be zero")
This could be worthy of reference?
mayachain/mayanode!69 (merged)
'[Version-unspecific] Observer item.Gas.NoneEmpty()'
mayachain/mayanode!69 (diffs)
I remember this was specifically for MAYAChain observing and not choking on THORChain zero-amount gas,
but it looks like it could be applicable to THORChain not choking on GAIA (or otherwise) zero-amount gas?
[/quote]
For this Issue I would rather like to hear thoughts/discussion on a preferred direction before any code writing.
(Replace Gas amount with minimum non-zero? Try to remove zero-amount gas altogether? Change validation behaviour to not reject zero amounts?)
Update:
Lacking impressions from others,
I have now submitted Merged Request !3126 (merged)
'NoneEmpty() for observed Gas too, not just Coins'
to call NoneEmpty()
for observed Gas alongside the existing call for observed Coins.
Edit:
This also appears relevant:
https://gitlab.com/thorchain/thornode/-/blob/v1.120.1/bifrost/pkg/chainclients/gaia/cosmos_block_scanner.go#L361-365
// THORChain only supports gas paid in ATOM, if gas is paid in another asset
// then fake gas as `0.000001 ATOM`, the fee is not used but cannot be empty
if gasFees.IsEmpty() {
gasFees = append(gasFees, common.NewCoin(c.cfg.ChainID.GetGasAsset(), cosmos.NewUint(1)))
}
For the zero-amount case, perhaps this is appending a second 1-amount Gas, but the remaining 0-amount Gas (which would be stripped out by NoneEmpty
) is causing the validation failure.
(Also, why is this code necessary, compared to only stripping out the empty Coin and having Gas be null?
Could the false-1-amount record be removed entirely without consequence?
(Perhaps in a later Merge Request rather than !3126 (merged), if so.))
[Edit:
This is what I had forgotten about (making the fake gas necessary), in Tx-received Valid()
!
https://gitlab.com/thorchain/thornode/-/blob/v1.120.2/common/tx.go#L203-205
if !tx.Chain.Equals(THORChain) && len(tx.Gas) == 0 {
return errors.New("must have at least 1 gas coin")
}
]