Add celo_list type to adapt the legacypool list type to fee currencies #34
Conversation
| nonce := tx.Nonce() | ||
| if removed := c.txs.Remove(nonce); !removed { | ||
| return false, nil | ||
| } | ||
| // Goes through celo_list subtotalcost to remove currency specific balances. | ||
| c.subTotalCost([]*types.Transaction{tx}) |
There was a problem hiding this comment.
First we remove a transaction with tx.Nonce(), then we reduce the cost by the cost of tx. This assumes that the removed transaction has the same cost as tx. Is this guaranteed?
There was a problem hiding this comment.
Yeah, it's not quite clear. If you check, the list.go impl does the same, which is why I implemented it this way just trying to hook the celo list subTotalCost. If you find references, you'll see in legacypool that the only usage of this method assumes (correctly I think) that they are both the same tx, same hash even, due to the invariances of the different structures. But from the standpoint of this type, it's definitely confusing.
core/txpool/legacypool/celo_list.go
Outdated
| for _, tx := range txs { | ||
| if txpool.IsFeeCurrencyTx(tx) { | ||
| feeCurrency := tx.FeeCurrency() | ||
| c.totalCost[*feeCurrency].Sub(c.totalCost[*feeCurrency], tx.Cost()) |
There was a problem hiding this comment.
What guarantees that the key exists? Similar to addTotalCost. Maybe worth extracting an internal function that would add costs to the correct value but would be called with -1 * tx.Cost() for subTotalCost and with just tx.Cost() for addTotalCost.
There was a problem hiding this comment.
I implemented it a different way, but with the same intention. Regardings your question, I believe is not possible to substract from a tx that hasn't been added before, and the currency keys never get deleted (which maybe they should). Still, check the new implementation.
| "github.com/ethereum/go-ethereum/core/types" | ||
| ) | ||
|
|
||
| type celo_list struct { |
There was a problem hiding this comment.
Why can't we extend the struct here? Is there a specific reason for that? We could remove "Forwarded Methods" altogether and shadow the remaining ones.
|
|
||
| // Tests that transactions can be added to strict lists and list contents and | ||
| // nonce boundaries are correctly maintained. | ||
| func TestStrictCeloListAdd(t *testing.T) { |
There was a problem hiding this comment.
I think we need more tests than this especially around total cost calculations.
There was a problem hiding this comment.
I agree, I was unsure to add those until the l1cost variables are clear
|
|
||
| // DenominateInCurrency returns the given value expressed in a different currency, according | ||
| // to the exchange rate at the state tree presented. | ||
| DenominateInCurrency(st *state.StateDB, fromNativeValue *big.Int, toFeeCurrency *common.Address) *big.Int |
There was a problem hiding this comment.
I think it would be better to just pass in the exchange rates here.
There was a problem hiding this comment.
my idea here is, in the impl of this fn to get the exchange rates from the state db and then use the fn from the branch you wrote to make the cal
| @@ -647,7 +647,7 @@ func (pool *LegacyPool) validateTx(tx *types.Transaction, local bool) error { | |||
| }, | |||
| ExistingExpenditure: func(addr common.Address) *big.Int { | |||
| if list := pool.pending[addr]; list != nil { | |||
| return list.totalcost | |||
| return list.TotalCost(tx) | |||
There was a problem hiding this comment.
This looks wrong, here we should check for the total cost of all pending transactions, not just the fee currency of tx. Or am I getting this worng?
There was a problem hiding this comment.
This is to check against the sender's balance for overdraft. So both the balance and the total cost has to be of the same currency as the tx being validated
|
|
||
| type celo_list struct { | ||
| list *list | ||
| totalCost map[common.Address]*big.Int |
There was a problem hiding this comment.
Have you thought about just making this change in the list struct? Maybe we wouldn't need the additional celo_list then? And some changes are required anyways because you have to adapt usage to TotalCost.
No description provided.