feat(state/core_accessor): add fee estimator #4087
Conversation
vgonkivs
added
area:state
vgonkivs
force-pushed
the
fee_estimator
branch
2 times, most recently
from
90f851c to
7c01a6f
Compare
vgonkivs
force-pushed
the
fee_estimator
branch
from
7c01a6f to
56afde4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4087 +/- ##
==========================================
+ Coverage 44.83% 45.14% +0.30%
==========================================
Files 265 309 +44
Lines 14620 22660 +8040
==========================================
+ Hits 6555 10229 +3674
- Misses 7313 11337 +4024
- Partials 752 1094 +342 ☔ View full report in Codecov by Sentry. |
vgonkivs
requested review from
renaynay,
Wondertan,
walldiss and
cristaloleg
as code owners
| func SanitizeAddr(addr string) (string, error) { | ||
| original := addr | ||
| // NormalizeAddress extracts the host and port, removing unsupported schemes. | ||
| func NormalizeAddress(addr string) string { |
There was a problem hiding this comment.
why does SanitizeAddr not work for this?
There was a problem hiding this comment.
it additionally cuts the port.
addr = strings.Split(addr, ":")[0]
| @@ -23,13 +24,14 @@ func coreAccessor( | |||
| fraudServ libfraud.Service[*header.ExtendedHeader], | |||
| network p2p.Network, | |||
| client *grpc.ClientConn, | |||
| address core.EstimatorAddress, | |||
There was a problem hiding this comment.
can't you just grab the core cfg there and take from cfg directly instead of supplying individual value to fx?
|
|
||
| log = logging.Logger("state") | ||
| ErrInvalidAmount = errors.New("state: amount must be greater than zero") | ||
| errGasPriceExceedsLimit = errors.New("state: estimated gasPrice exceeds max gasPrice") |
| if e.estimatorConn != nil && e.estimatorConn.GetState() != connectivity.Shutdown { | ||
| gasPrice, gas, err := signer.QueryGasUsedAndPrice(ctx, e.estimatorConn, priority.toApp(), rawTx) | ||
| if err == nil { | ||
| return gasPrice, gas, nil | ||
| } | ||
| log.Warn("failed to query gas used and price from the estimator endpoint.", "err", err) | ||
| } | ||
|
|
||
| if e.defaultClientConn == nil || e.defaultClientConn.GetState() == connectivity.Shutdown { | ||
| return 0, 0, errors.New("connection with the consensus node is dropped") | ||
| } | ||
|
|
||
| gasPrice, gas, err := signer.QueryGasUsedAndPrice(ctx, e.defaultClientConn, priority.toApp(), rawTx) | ||
| if err != nil { | ||
| log.Warn("state: failed to query gas used and price from the default endpoint", "err", err) | ||
| } |
There was a problem hiding this comment.
you can unify these to clean it up and just determine which conn to used based on if estimator conn exists.
What err will be returned if you pass a conn where connectivity shutdown to the signer.QueryGasUsedAndPrice? If it's readable (conn is closed) then we cna just return that unwrapped
| @@ -136,6 +139,7 @@ func (ca *CoreAccessor) Start(ctx context.Context) error { | |||
| if err != nil { | |||
| return fmt.Errorf("querying minimum gas price: %w", err) | |||
| } | |||
| ca.estimator.connect() | |||
There was a problem hiding this comment.
Start function could use some cleanup (better organisation w.r.t spacing + order)
This PR enables third-party estimation for the submit transactions in the node(besides PFB). By default, fee estimation relies on the consensus node to which the node is connected. But the user can now provide a non-default endpoint, so the node will query gas price and gas from it. It can be provided via cli(using "core.estimator.address"). The address will be added to the node's config as
FeeEstimatorAddress.Additionally, the user can now provide a maxGasPrice for every submit tx(via
max.gas.priceflag)- the max price that the user is willing to pay for the transaction. The transaction will not be submitted in case when estimated gas price exceeds the maxGasPrice(default:minGasPrice*100)