From 1ce0b7771970f8548d589b7d984e7bb0a886b4fa Mon Sep 17 00:00:00 2001 From: Maximilian Langenfeld <15726643+ezdac@users.noreply.github.com> Date: Tue, 11 Jun 2024 12:15:57 +0200 Subject: [PATCH] Allow non-hash based blocknum argument in CeloAPI backend (#138) * Allow non-hash based blocknum argument in CeloAPI backend * Remove LRU cache from GetExchangeRates Lets keep it simple and not do any premature optimization. If we implement this it might be better to do it on a lower level anyways. --- internal/celoapi/backend.go | 42 ++++++++---------------- internal/ethapi/api.go | 10 +++--- internal/ethapi/api_test.go | 10 +++--- internal/ethapi/backend.go | 8 ++--- internal/ethapi/transaction_args.go | 28 +++++++++++++--- internal/ethapi/transaction_args_test.go | 8 ++--- 6 files changed, 55 insertions(+), 51 deletions(-) diff --git a/internal/celoapi/backend.go b/internal/celoapi/backend.go index 89aa0b845a..3333dcc03b 100644 --- a/internal/celoapi/backend.go +++ b/internal/celoapi/backend.go @@ -7,7 +7,6 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common/exchange" - "github.com/ethereum/go-ethereum/common/lru" "github.com/ethereum/go-ethereum/contracts" "github.com/ethereum/go-ethereum/internal/ethapi" "github.com/ethereum/go-ethereum/rpc" @@ -15,8 +14,7 @@ import ( func NewCeloAPIBackend(b ethapi.Backend) *CeloAPIBackend { return &CeloAPIBackend{ - Backend: b, - exchangeRatesCache: lru.NewCache[common.Hash, common.ExchangeRates](128), + Backend: b, } } @@ -24,26 +22,17 @@ func NewCeloAPIBackend(b ethapi.Backend) *CeloAPIBackend { // functionality. CeloAPIBackend is mainly passed to the JSON RPC services and provides // an easy way to make extra functionality available in the service internal methods without // having to change their call signature significantly. -// CeloAPIBackend keeps a threadsafe LRU cache of block-hash to exchange rates for that block. -// Cache invalidation is only a problem when an already existing blocks' hash -// doesn't change, but the rates change. That shouldn't be possible, since changing the rates -// requires different transaction hashes / state and thus a different block hash. -// If the previous rates change during a reorg, the previous block hash should also change -// and with it the new block's hash. -// Stale branches cache values will get evicted eventually. type CeloAPIBackend struct { ethapi.Backend - - exchangeRatesCache *lru.Cache[common.Hash, common.ExchangeRates] } -func (b *CeloAPIBackend) getContractCaller(ctx context.Context, atBlock common.Hash) (*contracts.CeloBackend, error) { +func (b *CeloAPIBackend) getContractCaller(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash) (*contracts.CeloBackend, error) { state, _, err := b.Backend.StateAndHeaderByNumberOrHash( ctx, - rpc.BlockNumberOrHashWithHash(atBlock, false), + blockNumOrHash, ) if err != nil { - return nil, fmt.Errorf("retrieve state for block hash %s: %w", atBlock.String(), err) + return nil, fmt.Errorf("retrieve state for block hash %s: %w", blockNumOrHash.String(), err) } return &contracts.CeloBackend{ ChainConfig: b.Backend.ChainConfig(), @@ -51,41 +40,36 @@ func (b *CeloAPIBackend) getContractCaller(ctx context.Context, atBlock common.H }, nil } -func (b *CeloAPIBackend) GetFeeBalance(ctx context.Context, atBlock common.Hash, account common.Address, feeCurrency *common.Address) (*big.Int, error) { - cb, err := b.getContractCaller(ctx, atBlock) +func (b *CeloAPIBackend) GetFeeBalance(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash, account common.Address, feeCurrency *common.Address) (*big.Int, error) { + cb, err := b.getContractCaller(ctx, blockNumOrHash) if err != nil { return nil, err } return contracts.GetFeeBalance(cb, account, feeCurrency), nil } -func (b *CeloAPIBackend) GetExchangeRates(ctx context.Context, atBlock common.Hash) (common.ExchangeRates, error) { - cachedRates, ok := b.exchangeRatesCache.Get(atBlock) - if ok { - return cachedRates, nil - } - cb, err := b.getContractCaller(ctx, atBlock) +func (b *CeloAPIBackend) GetExchangeRates(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash) (common.ExchangeRates, error) { + contractBackend, err := b.getContractCaller(ctx, blockNumOrHash) if err != nil { return nil, err } - er, err := contracts.GetExchangeRates(cb) + er, err := contracts.GetExchangeRates(contractBackend) if err != nil { return nil, err } - b.exchangeRatesCache.Add(atBlock, er) return er, nil } -func (b *CeloAPIBackend) ConvertToCurrency(ctx context.Context, atBlock common.Hash, value *big.Int, fromFeeCurrency *common.Address) (*big.Int, error) { - er, err := b.GetExchangeRates(ctx, atBlock) +func (b *CeloAPIBackend) ConvertToCurrency(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash, value *big.Int, fromFeeCurrency *common.Address) (*big.Int, error) { + er, err := b.GetExchangeRates(ctx, blockNumOrHash) if err != nil { return nil, err } return exchange.ConvertCeloToCurrency(er, fromFeeCurrency, value) } -func (b *CeloAPIBackend) ConvertToGold(ctx context.Context, atBlock common.Hash, value *big.Int, toFeeCurrency *common.Address) (*big.Int, error) { - er, err := b.GetExchangeRates(ctx, atBlock) +func (b *CeloAPIBackend) ConvertToCelo(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash, value *big.Int, toFeeCurrency *common.Address) (*big.Int, error) { + er, err := b.GetExchangeRates(ctx, blockNumOrHash) if err != nil { return nil, err } diff --git a/internal/ethapi/api.go b/internal/ethapi/api.go index b9ff9361ad..2269abbf40 100644 --- a/internal/ethapi/api.go +++ b/internal/ethapi/api.go @@ -1315,9 +1315,9 @@ func DoEstimateGas(ctx context.Context, b CeloBackend, args TransactionArgs, blo // Celo specific: get exchange rates if fee currency is specified exchangeRates := emptyExchangeRates if args.FeeCurrency != nil { - // It is debatable whether we should use Hash or ParentHash here. Usually, - // user would probably like the recent rates after the block, so we use Hash. - exchangeRates, err = b.GetExchangeRates(ctx, header.Hash()) + // It is debatable whether we should use the block itself or the parent block here. + // Usually, user would probably like the recent rates after the block, so we use the block itself. + exchangeRates, err = b.GetExchangeRates(ctx, blockNrOrHash) if err != nil { return 0, fmt.Errorf("get exchange rates for block: %v err: %w", header.Hash(), err) } @@ -1326,7 +1326,7 @@ func DoEstimateGas(ctx context.Context, b CeloBackend, args TransactionArgs, blo call := args.ToMessage(header.BaseFee, exchangeRates) // Celo specific: get balance - balance, err := b.GetFeeBalance(ctx, opts.Header.Hash(), call.From, args.FeeCurrency) + balance, err := b.GetFeeBalance(ctx, blockNrOrHash, call.From, args.FeeCurrency) if err != nil { return 0, err } @@ -1752,7 +1752,7 @@ func AccessList(ctx context.Context, b CeloBackend, blockNrOrHash rpc.BlockNumbe // Always use the header's parent here, since we want to create the list at the // queried block, but want to use the exchange rates before (at the beginning of) // the queried block - exchangeRates, err = b.GetExchangeRates(ctx, header.ParentHash) + exchangeRates, err = b.GetExchangeRates(ctx, rpc.BlockNumberOrHashWithHash(header.ParentHash, false)) if err != nil { return nil, 0, nil, fmt.Errorf("get exchange rates for block: %v err: %w", header.Hash(), err) } diff --git a/internal/ethapi/api_test.go b/internal/ethapi/api_test.go index 4ed8a30354..08dc68121d 100644 --- a/internal/ethapi/api_test.go +++ b/internal/ethapi/api_test.go @@ -590,9 +590,9 @@ type celoTestBackend struct { *testBackend } -func (c *celoTestBackend) GetFeeBalance(ctx context.Context, atBlock common.Hash, account common.Address, feeCurrency *common.Address) (*big.Int, error) { +func (c *celoTestBackend) GetFeeBalance(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash, account common.Address, feeCurrency *common.Address) (*big.Int, error) { if feeCurrency == nil { - header, err := c.HeaderByHash(ctx, atBlock) + header, err := c.HeaderByNumberOrHash(ctx, blockNumOrHash) if err != nil { return nil, fmt.Errorf("retrieve header by hash in testBackend: %w", err) } @@ -607,12 +607,12 @@ func (c *celoTestBackend) GetFeeBalance(ctx context.Context, atBlock common.Hash return nil, errCeloNotImplemented } -func (c *celoTestBackend) GetExchangeRates(ctx context.Context, atBlock common.Hash) (common.ExchangeRates, error) { +func (c *celoTestBackend) GetExchangeRates(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash) (common.ExchangeRates, error) { var er common.ExchangeRates return er, nil } -func (c *celoTestBackend) ConvertToCurrency(ctx context.Context, atBlock common.Hash, value *big.Int, feeCurrency *common.Address) (*big.Int, error) { +func (c *celoTestBackend) ConvertToCurrency(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash, value *big.Int, feeCurrency *common.Address) (*big.Int, error) { if feeCurrency == nil { return value, nil } @@ -620,7 +620,7 @@ func (c *celoTestBackend) ConvertToCurrency(ctx context.Context, atBlock common. return nil, errCeloNotImplemented } -func (c *celoTestBackend) ConvertToGold(ctx context.Context, atBlock common.Hash, value *big.Int, feeCurrency *common.Address) (*big.Int, error) { +func (c *celoTestBackend) ConvertToCelo(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash, value *big.Int, feeCurrency *common.Address) (*big.Int, error) { if feeCurrency == nil { return value, nil } diff --git a/internal/ethapi/backend.go b/internal/ethapi/backend.go index a8f13ae94b..4048aa2b96 100644 --- a/internal/ethapi/backend.go +++ b/internal/ethapi/backend.go @@ -40,10 +40,10 @@ import ( type CeloBackend interface { Backend - GetFeeBalance(ctx context.Context, atBlock common.Hash, account common.Address, feeCurrency *common.Address) (*big.Int, error) - GetExchangeRates(ctx context.Context, atBlock common.Hash) (common.ExchangeRates, error) - ConvertToCurrency(ctx context.Context, atBlock common.Hash, value *big.Int, feeCurrency *common.Address) (*big.Int, error) - ConvertToGold(ctx context.Context, atBlock common.Hash, value *big.Int, feeCurrency *common.Address) (*big.Int, error) + GetFeeBalance(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash, account common.Address, feeCurrency *common.Address) (*big.Int, error) + GetExchangeRates(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash) (common.ExchangeRates, error) + ConvertToCurrency(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash, value *big.Int, feeCurrency *common.Address) (*big.Int, error) + ConvertToCelo(ctx context.Context, blockNrOrHash rpc.BlockNumberOrHash, value *big.Int, feeCurrency *common.Address) (*big.Int, error) } // Backend interface provides the common API services (that are provided by both full and light clients) with access to necessary functions. diff --git a/internal/ethapi/transaction_args.go b/internal/ethapi/transaction_args.go index b6a7e7c17f..76f4048156 100644 --- a/internal/ethapi/transaction_args.go +++ b/internal/ethapi/transaction_args.go @@ -248,7 +248,12 @@ func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b CeloBackend) return err } if args.IsFeeCurrencyDenominated() { - price, err = b.ConvertToCurrency(ctx, head.Hash(), price, args.FeeCurrency) + price, err = b.ConvertToCurrency( + ctx, + rpc.BlockNumberOrHashWithHash(head.Hash(), false), + price, + args.FeeCurrency, + ) if err != nil { return fmt.Errorf("can't convert suggested gasTipCap to fee-currency: %w", err) } @@ -272,7 +277,12 @@ func (args *TransactionArgs) setCancunFeeDefaults(ctx context.Context, head *typ // wether the blob-fee will be used like that in Cel2 or not, // at least this keeps it consistent with the rest of the gas-fees var err error - blobBaseFee, err = b.ConvertToCurrency(ctx, head.Hash(), blobBaseFee, args.FeeCurrency) + blobBaseFee, err = b.ConvertToCurrency( + ctx, + rpc.BlockNumberOrHashWithHash(head.Hash(), false), + blobBaseFee, + args.FeeCurrency, + ) if err != nil { return fmt.Errorf("can't convert blob-fee to fee-currency: %w", err) } @@ -295,7 +305,12 @@ func (args *TransactionArgs) setLondonFeeDefaults(ctx context.Context, head *typ return err } if args.IsFeeCurrencyDenominated() { - tip, err = b.ConvertToCurrency(ctx, head.Hash(), tip, args.FeeCurrency) + tip, err = b.ConvertToCurrency( + ctx, + rpc.BlockNumberOrHashWithHash(head.Hash(), false), + tip, + args.FeeCurrency, + ) if err != nil { return fmt.Errorf("can't convert suggested gasTipCap to fee-currency: %w", err) } @@ -310,7 +325,12 @@ func (args *TransactionArgs) setLondonFeeDefaults(ctx context.Context, head *typ baseFee := head.BaseFee if args.IsFeeCurrencyDenominated() { var err error - baseFee, err = b.ConvertToCurrency(ctx, head.Hash(), baseFee, args.FeeCurrency) + baseFee, err = b.ConvertToCurrency( + ctx, + rpc.BlockNumberOrHashWithHash(head.Hash(), false), + baseFee, + args.FeeCurrency, + ) if err != nil { return fmt.Errorf("can't convert base-fee to fee-currency: %w", err) } diff --git a/internal/ethapi/transaction_args_test.go b/internal/ethapi/transaction_args_test.go index ef242672e7..02a0dd3782 100644 --- a/internal/ethapi/transaction_args_test.go +++ b/internal/ethapi/transaction_args_test.go @@ -264,23 +264,23 @@ func newCeloBackendMock() *celoBackendMock { } } -func (c *celoBackendMock) GetFeeBalance(ctx context.Context, atBlock common.Hash, account common.Address, feeCurrency *common.Address) (*big.Int, error) { +func (c *celoBackendMock) GetFeeBalance(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash, account common.Address, feeCurrency *common.Address) (*big.Int, error) { // Celo specific backend features are currently not tested return nil, errCeloNotImplemented } -func (c *celoBackendMock) GetExchangeRates(ctx context.Context, atBlock common.Hash) (common.ExchangeRates, error) { +func (c *celoBackendMock) GetExchangeRates(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash) (common.ExchangeRates, error) { var er common.ExchangeRates // Celo specific backend features are currently not tested return er, errCeloNotImplemented } -func (c *celoBackendMock) ConvertToCurrency(ctx context.Context, atBlock common.Hash, value *big.Int, fromFeeCurrency *common.Address) (*big.Int, error) { +func (c *celoBackendMock) ConvertToCurrency(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash, value *big.Int, fromFeeCurrency *common.Address) (*big.Int, error) { // Celo specific backend features are currently not tested return nil, errCeloNotImplemented } -func (c *celoBackendMock) ConvertToGold(ctx context.Context, atBlock common.Hash, value *big.Int, toFeeCurrency *common.Address) (*big.Int, error) { +func (c *celoBackendMock) ConvertToCelo(ctx context.Context, blockNumOrHash rpc.BlockNumberOrHash, value *big.Int, toFeeCurrency *common.Address) (*big.Int, error) { // Celo specific backend features are currently not tested return nil, errCeloNotImplemented }