Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply TxFeeCap conversion #321

Open
wants to merge 6 commits into
base: celo11
Choose a base branch
from

Conversation

Kourin1996
Copy link

@Kourin1996 Kourin1996 commented Feb 5, 2025

Closes #313

This PR adds fee cap conversion in CheckTxFee. ConvertTxFeeCapToCurrency fetches TxFeeCap from backend and convert it to a tx fee currency.

@Kourin1996 Kourin1996 self-assigned this Feb 5, 2025
@Kourin1996 Kourin1996 requested review from piersy and ezdac February 5, 2025 13:56
Copy link

@ezdac ezdac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me, however I don't understand why you converted the float multiplier instead of the gas-price.

internal/ethapi/api.go Outdated Show resolved Hide resolved
internal/ethapi/api.go Outdated Show resolved Hide resolved
internal/ethapi/celo_api_test.go Outdated Show resolved Hide resolved
internal/ethapi/celo_api_test.go Outdated Show resolved Hide resolved
@Kourin1996 Kourin1996 requested a review from ezdac February 11, 2025 04:51
Copy link

@ezdac ezdac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one remaining nitpick from my side, otherwise it's good to go now!

rate, ok := rates[*feeCurrency]
if !ok {
return 0, fmt.Errorf("could not convert from native to fee currency (fee-currency=%s): %w ", feeCurrency, exchange.ErrUnregisteredFeeCurrency)
log.Warn("Failed to get exchange rates for latest block", "err", err)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is overly verbose - especially since this warning can't be correlated to the request arguments and failed RPC requests are already logged with additional request-context:

op-geth/rpc/handler.go

Lines 476 to 481 in 055f61b

if resp.Error != nil {
logctx = append(logctx, "err", resp.Error.Message)
if resp.Error.Data != nil {
logctx = append(logctx, "errdata", formatErrorData(resp.Error.Data))
}
h.log.Warn("Served "+msg.Method, logctx...)

So I'd just pass along the error, or wrap the error when the additional message should also be visible to the caller in the response.

return 0, fmt.Errorf("failed to accurately convert fee currency to float64")
gasPriceInCelo, err := exchange.ConvertCurrencyToCelo(rates, feeCurrency, gasPrice)
if err != nil {
log.Warn("Failed to convert gas price", "err", err, "currency", feeCurrency)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see other comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check fee currency handling in checkTxFee
2 participants