chore(wallet)_: fee handling improvements for Status Network Sepolia #6710
Conversation
saledjenic
requested review from
a team,
dlipicar,
Khushboo-dev-cpp,
alaibe and
vkjr
and removed request for
a team
Jenkins BuildsClick to see older builds (80)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6710 +/- ##
===========================================
- Coverage 59.18% 59.18% -0.01%
===========================================
Files 828 828
Lines 101465 101546 +81
===========================================
+ Hits 60052 60097 +45
- Misses 33836 33865 +29
- Partials 7577 7584 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| if common.IsGaslessChainAndEIP1559Compatible(chainID) { | ||
| return true, nil | ||
| // Since the Status Network is gasless chain, but EIP-1559 compatible, we should not rely on checking the BaseFeePerGas, that's why we have this special case. | ||
| eip1559Enabled, err := common.IsPartiallyOrFullyGaslessChainEIP1559Compatible(chainID) |
There was a problem hiding this comment.
IsPartiallyOrFullyGaslessChainEIP1559Compatible function looks like overcomplication for me. If all we want to check is that chain is EIP1559-compatible, we don't have to bring the concept of "gasless" into play. We can have function common. IsKnownEIP1559CompatibleChain and use it instead and put the check for status chain there.
There was a problem hiding this comment.
@vkjr not really. I prefer naming things what they truly are and what they truly do.
If all we want to check is that chain is EIP1559-compatible
Yes, the function IsEIP1559Enabled really checks if a chain X is EIP1559 compatible or not, and it doesn't bring "gasless" into play, and works for any chain. But the logic for checking that for gasless chains and partially gasless chains differs from checking that for other chains.
We can have function common. IsKnownEIP1559CompatibleChain
Why do we need that one if we already have IsEIP1559Enabled that does that?
There was a problem hiding this comment.
Why do we need that one if we already have IsEIP1559Enabled that does that?
Well, right now IsEIP1559Enabled calls IsPartiallyOrFullyGaslessChainEIP1559Compatible which calls !IsPartiallyOrFullyGaslessChain and all these nested calls do is check if chainId is Status Sepolia. Just putting this check inside isEIP1559Compatible would make things more readable.
There was a problem hiding this comment.
Buy saying "bring the concept of "gasless" into play" I mean that the word "gasless" appears inside the IsEIP1559Enabled function when we call IsPartiallyOrFullyGaslessChainEIP1559Compatible.
So for someone like me who is less aware of a codebase and reads the code of IsEIP1559Enabled that poses a question "what gasless have to do with EIP 1559", so I'm talking more about cognitive load of reading the code here :)
There was a problem hiding this comment.
Even it's a simple function:
func IsPartiallyOrFullyGaslessChain(chainID uint64) bool {
return chainID == StatusNetworkSepolia
}it says a lot. Has a meaningful name, is used in more than a single place in the code, and having in mind that tomorrow when we need to add StatusMainnetChain (or maybe some other gasless chain) that will be just a single line change in that function, it deserves to be a separate function.
There was a problem hiding this comment.
Just because of the discussion that you ran here, which I wanted to avoid, and don't waste time, I added this comment
// This is something we can do just now (cause Lenea call has some issues), but should not be used later (after September 2025),
// instead `linea_estimateGas` call should be used for the Status Network.
at the moment when I added that condition there.
There was a problem hiding this comment.
@saledjenic, I've heard that this condition will go away "soon". And I don't see how this as a reason for not improving code "now". Because we all know how delayed can be expected changes.
Also, even if initialization to zeros will go away, where exactly will reside the receiving of actual values with linea_estimateGas? Right now I see 2 functions within SuggestedFees: getEIP1559SuggestedFees and getNonEIP1559SuggestedFees. Receiving fees won't be in any of them?
Speaking of hearing each other. Could you please comment on the whole point of having if chainID == common.StatusNetworkSepolia check within SuggestedFees. As I noted few times, all it does is initializing nil values to zeros. And we do not treat those values any different. What is the point?
There was a problem hiding this comment.
And I don't see how this as a reason for not improving code "now".
Cause that's a temporary code, it doesn't hurt, but even if we don't change it in 3 months, but in 6 months for example, it will boil down to 3 lines (condition) removal and that's it, the rest of the code is untouched and clean. Also that's the point of the comment there.
Right now I see 2 functions within SuggestedFees: getEIP1559SuggestedFees and getNonEIP1559SuggestedFees. Receiving fees won't be in any of them?
Ofc not, they receive feeHistory as an input param and do the calculation.
Could you please comment on the whole point of having if chainID == common.StatusNetworkSepolia check within SuggestedFees.
There are only 2 places where we have if chainID == common.StatusNetworkSepolia, and those are:
- in
SuggestedFees - in
getEIP1559SuggestedFees
We ignore the second one cause it's temporary here and will go away soon. That's just a patch for the Status chain. As soon as the fix lands in the Status chain, we can remove it.
In the first one remains to differentiate Status chain from other chains cause it fetches details from linea_estimateGas, to be clear, the condition could go to getEIP1559SuggestedFees, but then the function would need to be a member of FeeManager.
There was a problem hiding this comment.
There are only 2 places where we have if chainID == common.StatusNetworkSepolia, and those are:
- in SuggestedFees
- in getEIP1559SuggestedFees
We ignore the second one cause it's temporary here and will go away soon. That's just a patch for the Status chain. As soon as the fix lands in the Status chain, we can remove it.
I'm fully okay with this one, I think it fully covers initialization needs for now.
In the first one remains to differentiate Status chain from other chains cause it fetches details from linea_estimateGas, to be clear, the condition could go to getEIP1559SuggestedFees, but then the function would need to be a member of FeeManager.
This one I think should be removed because at the moment we don't have linea_estimateGas and the whole if condition doesn't make anything functional. Receiving noBaseFee and noPriorityFee within it also has no practical sense, it only pollutes SuggestedFees returned values and can be done outside of SuggestedFees.
There was a problem hiding this comment.
This one I think should be removed because at the moment we don't have linea_estimateGas
How do you mean that? It must stay and we have to use linea_estimateGas. The only question is, will we keep the call in SuggestedFees or move it to getEIP1559SuggestedFees and make that function a member of FeeManager. Those are only 2 possible options and no other way.
| @@ -47,9 +50,36 @@ func (f *FeeManager) getFeeHistory(ctx context.Context, chainID uint64, newestBl | |||
| return feeHistory, err | |||
| } | |||
|
|
|||
| func (f *FeeManager) getGaslessParamsForAccount(ctx context.Context, chainID uint64, address ethCommon.Address) (noBaseFee bool, noPriorityFee bool, err error) { | |||
| if !walletCommon.IsPartiallyOrFullyGaslessChain(chainID) { | |||
| return false, false, nil | |||
There was a problem hiding this comment.
Should this be an error condition?
There was a problem hiding this comment.
No, it doesn't. Why do you think so?
There was a problem hiding this comment.
I noticed that we call this function on the top level under condition chainID == common.StatusNetworkSepolia, so it looked like it is intended to be called for gasless chains.
There was a problem hiding this comment.
Yes we call it under that condition, but that's our need in SuggestedFees function, but this function works for any chain supported in the app, not only for StatusNetworkSepolia chain.
There was a problem hiding this comment.
Okay, in this function we do check IsPartiallyOrFullyGaslessChain but it should work for any network, got it 👍
At the same time in SuggestedFees we firstly check if chainId is status sepolia to initialize fees to zeros. I have a feeling that the same logic should work for any gasless chain, not only for status sepolia.
So it probably would be more logical to cal the getGaslessParamsForAccount first and then depending on returned values of noBaseFee and noPriorityFee initialize fee values with zeros or with the result of getEIP1559SuggestedFees. In this way we will avoid manual check for sepolia status chain id and will rely on more generic IsPartiallyOrFullyGaslessChain used within getGaslessParamsForAccount.
Wdyt?
There was a problem hiding this comment.
Yes, I agree, but isn't this more optimised :) like in that case call to getGaslessParamsForAccount and the code between 145 and 159 lines would be done for every chain, instead of as it's now, that we call that function only if it's StatusSepoliaChain. In all cases we need feeHistory, but getGaslessParamsForAccount are needed only in case of StautsSepoliaChain.
There was a problem hiding this comment.
I lean toward the opinion that preliminary optimization is evil and personally always prefer code simplicity unless cpu or memory profiler said I'm wrong)
But I see your point, that also doesn't look beautiful and I think now I may understand why.. because we actually already have a function that should compute all fees - getEIP1559SuggestedFees. This function inside already has a check for status sepolia (line 79)! And on top of it we do this check again and set some fees to zero in `SuggestedFees.
I think we may merge getEIP1559SuggestedFees and f.getGaslessParamsForAccount which will make the SuggestedFees much cleaner.
Also getEIP1559SuggestedFees can return the whole populated suggestedFees structure, it has all data to do this.
wdyt?
There was a problem hiding this comment.
Yes, that's cleaner.
@vkjr actually not, that's not good, cause everything done in this PR has its own reason why it's like that and I would keep it as it is. We cannot merge it cause moving getGaslessParamsForAccount into getEIP1559SuggestedFees will cause other not that nice changes, getGaslessParamsForAccount is a member of FeeManager, then we would need to pass context and address to getEIP1559SuggestedFees which is needless and also acting on the error returned by is is not the same. Those are different cases. So no changes on this topic.
There was a problem hiding this comment.
Looks like I initially posted this in a wrong theread. Comment should be in this one.
@saledjenic, could you please help me understand the difference in these few cases, I'm not sure I correctly grasp them, maybe because of lack of Go knowledge.
Let's say we are in SuggestedFees.
We have baseFee *big.Int (which will be inialized to nil at first)
Case 1: Network is Status Sepolia and noBaseFee returned from f.getGaslessParamsForAccount equals true.
In this case we will get following sequence:
- we are entering
if chainID == common.StatusNetworkSepolia noBaseFeeequals truebaseFee = big.NewInt(0)//doing conditional initialization- we are exiting
ifblock BaseFee: baseFee//suggestedFees.BaseFee initialized with 0
Case 2: Network is Status Sepolia and noBaseFee returned from f.getGaslessParamsForAccount equals false.
- we are entering
if chainID == common.StatusNetworkSepolia noBaseFeeequals false- we are exiting
ifblock BaseFee: baseFee//suggestedFees.BaseFee will remainnil
Case 3, Hypothetical: Let's say network is Status Sepolia but we don't have a call to f.getGaslessParamsForAccount in code and only call f.getEIP1559SuggestedFees. In this case:
- we enter
getEIP1559SuggestedFees if chainID == common.StatusNetworkSepolia {//condition meetsreturn big.NewInt(0), big.NewInt(0), big.NewInt(0), big.NewInt(0), nil// we return 0 values- exit
getEIP1559SuggestedFees BaseFee: baseFee//suggestedFees.BaseFee initialized with 0
As we can see all these 3 cases leave us with the same result as for fees. Base fee will be 0 or nil which is indifferent in this case (correct me if I'm wrong) because we are never check for nil later.
So that means that call to f.getGaslessParamsForAccount has no practical value for calculating fees.
As you have said earlier,noBaseFee and noPriorityFee are the parameters of chain.
So instead of calling f.getGaslessParamsForAccount inside SuggestedFees (which doesn't affect fees at all) and dragging two fee-unrelated params out of SuggestedFees to r.evaluateAndUpdatePathDetails, we can get rid of that call inside SuggestedFees at all and rely only on what f.getEIP1559SuggestedFees returns, while the call to f.getGaslessParamsForAccount can be moved to evaluateAndUpdatePathDetails. Or maybe it is not needed inside evaluateAndUpdatePathDetails but can be placed before it, on the same level as SuggestedFees.
What we will get:
- no double check for sepolia status in
SuggestedFeesand inf.getEIP1559SuggestedFees - no need to initialize
&SuggestedFeesstructure inSuggestedFeesfunc, it can be done insidef.getEIP1559SuggestedFees - no dragging parameters
I think that would be much cleaner, wdyt?
| @@ -125,18 +128,44 @@ func (f *FeeManager) IsEIP1559Enabled(ctx context.Context, chainID uint64) (bool | |||
| return block.BaseFee() != nil && block.BaseFee().Cmp(big.NewInt(0)) > 0, nil | |||
| } | |||
|
|
|||
| func (f *FeeManager) SuggestedFees(ctx context.Context, chainID uint64) (*SuggestedFees, error) { | |||
| func (f *FeeManager) SuggestedFees(ctx context.Context, chainID uint64, address ethCommon.Address) (suggestedFees *SuggestedFees, noBaseFee bool, noPriorityFee bool, err error) { | |||
There was a problem hiding this comment.
noBaseFee and noPriorityFee are never used on their own, they always passed to other functions along with suggestedFees. In r.evaluateAndUpdatePathDetails, in r.buildPath, which makes them logically a part of SuggestedFees structure.
So we can move these 2 fields inside SuggestedFees and avoid 2 extra params
There was a problem hiding this comment.
@vkjr yes, I was thinking about that initially, butSuggestedFees type carries the info about fees, how much is each fee, base fee, priority fee... but if there is a base fee or not and if there is a priority fee or not are more fields that are describing the chain behaviour. So the chain can be base-feeless and/or priority-feeless and if the chan has no base fee baseFee field in the SuggestedFees type will be 0, the same logic for priority fee. So that's the reason why they are not there.
There was a problem hiding this comment.
That makes sense.
If noBaseFee and noPriorityFee are used to update chain fields, can we just carry this information using SuggestedFess structure?
For example right now inside SuggestedFees function we are getting noBaseFee and noPriorityFee:
noBaseFee, noPriorityFee, err = f.getGaslessParamsForAccount(ctx, chainID, address)
And use them:
if noBaseFee {
baseFee = big.NewInt(0)
}
But we can also use nil value assigned to baseFee that can indicate that the chain doesn't have baseFee at all. That would result in:
if noBaseFee {
baseFee = nil
}
And we won't have to pass 2 extra params everywhere. Wdyt?
There was a problem hiding this comment.
Yes, technically we could do that, but I wanted to follow safer solution, cause that one we would most likely require clients updates, cause I am not sure if clients are handling properly null values for those fields and most likely we would need many more lines in functional and unit tests to update.
saledjenic
force-pushed
the
chore/router-simplification
branch
from
2fab779 to
824b15a
Compare
saledjenic
force-pushed
the
chore/status-chain-gasless-related-improvements
branch
from
305cdb5 to
fecfc8d
Compare
saledjenic
force-pushed
the
chore/router-simplification
branch
from
824b15a to
6971560
Compare
saledjenic
force-pushed
the
chore/status-chain-gasless-related-improvements
branch
from
fecfc8d to
5670e91
Compare
| @@ -655,6 +657,30 @@ func (c *ClientWithFallback) FeeHistory(ctx context.Context, blockCount uint64, | |||
| } | |||
|
|
|||
| func (c *ClientWithFallback) EstimateGas(ctx context.Context, msg ethereum.CallMsg) (uint64, error) { | |||
| if c.ChainID == walletCommon.StatusNetworkSepolia { | |||
There was a problem hiding this comment.
-
Can you move this part (
if chainID == StatusNetworkSepolia) to a separate method.
I think this violates the Open/Closed Principle from SOLID (open for extension but closed for modification). It adds complexity without any benefits. Harder to read, maintain. -
StatusNetworkSepoliaChainIDis also defined inconst.go
There was a problem hiding this comment.
StatusNetworkSepoliaChainID is also defined in const.go
One from wallet constants should be used, I can add a commit on top of with that change
Can you move this part (if chainID == StatusNetworkSepolia) to a separate method.
@friofry I can, but in that case this EstimateGas doesn't work for Status Chain, I mean, doesn't give the correct value.
There was a problem hiding this comment.
doesn't give the correct value.
I guess it's fine. I think this business logic is not the matter of RPC connection wrapper
There was a problem hiding this comment.
@friofry new commit added for constants.
There was a problem hiding this comment.
@friofry it's not a quick thing to split that here, cause our processors are estimating gas via GasEstimator interface of go-ethereum, and I would need to update our fork to make it different, which I don't want, since we want to make changes on our side that will let us update easily to the latest go-ethereum implementation.
Need to think and check the code on how to handle that on our side, but that will definitely be a new PR.
The Base chain is based on Optimism stack, so should follow the same fee calculation.
- Remove unused functions `HasNoBaseFee` and `HasNoPriorityFee` from wallet utilities since that should not be hardcoded, but dynamically evaluated. - A temporary patch was added to the `getEIP1559SuggestedFees` function to handle the specificities of the Status chain.
saledjenic
force-pushed
the
chore/status-chain-gasless-related-improvements
branch
2 times, most recently
from
d87ba4b to
83ceb96
Compare
- Added support for the `linea_estimateGas` method in the wallet's gas estimation logic. - Updated the `EstimateGas` function to route to `linea_estimateGas` when the chain ID is for Status Sepolia chain. - Enhanced fee management to dynamically evaluate the account that is going to place a tx is base fee less and/or priority fee less for the Status Sepolia chain.
saledjenic
force-pushed
the
chore/status-chain-gasless-related-improvements
branch
from
83ceb96 to
9e7c21c
Compare
saledjenic
force-pushed
the
chore/status-chain-gasless-related-improvements
branch
from
9e7c21c to
ed44455
Compare
…tants from wallet common are used instead - Removed api `common` package since except in a few places the app is using chain constants from the wallet `common` package - All references to chain IDs from removed package are updated
saledjenic
force-pushed
the
chore/status-chain-gasless-related-improvements
branch
from
ed44455 to
34394dd
Compare
HasNoBaseFeeandHasNoPriorityFeefrom wallet utilities since that should not be hardcoded, but dynamically evaluated.getEIP1559SuggestedFeesfunction to handle the specificities of the Status chain.The last commit bring a dynamic fee evaluation for Status Sepolia chain:
linea_estimateGasmethod in the wallet's gas estimation logic.EstimateGasfunction to route tolinea_estimateGaswhen the chain ID is for Status Sepolia chain.and/or priority fee less for the Status Sepolia chain.