-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: l1 gas price estimation when placing l2 transaction #4923
Conversation
Jenkins BuildsClick to see older builds (24)
|
d4dc656
to
4067f22
Compare
d9bd863
to
a2383ca
Compare
services/wallet/router.go
Outdated
@@ -547,6 +549,48 @@ func (r *Router) getERC1155Balance(ctx context.Context, network *params.Network, | |||
return balances[0].Int, nil | |||
} | |||
|
|||
func (r *Router) getL1Fee(ctx context.Context, network *params.Network, fromAddress common.Address, toAddress common.Address, amountIn *big.Int) (uint64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested sending erc20 and sending eth from metamask and there is differences in l1 fees.
I beleve this logic is only valid for the case of sending eth, there should be logic for all types of tx:
Bridge / buy ENS, send ERC20, sendERC71.
This logic should be encapsulated by bridge
services/wallet/router.go
Outdated
@@ -737,6 +792,7 @@ func (r *Router) suggestedRoutes( | |||
AmountIn: (*hexutil.Big)(zero), | |||
AmountOut: (*hexutil.Big)(zero), | |||
GasAmount: gasLimit, | |||
L1GasAmount: l1Fee, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as said above, this should directly be attached to the gasfees
services/wallet/router.go
Outdated
// Removed the required fees from maxAMount in case of native token tx | ||
if token.IsNative() { | ||
maxAmountIn = (*hexutil.Big)(new(big.Int).Sub(maxAmountIn.ToInt(), requiredNativeBalance)) | ||
approvalRequired, approvalAmountRequired, approvalGasLimit, approvalContractAddress, err := r.requireApproval(ctx, sendType, bridge, addrFrom, network, token, amountIn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require approval should also have a l1 fees dedicated value
48d0dd3
to
6e30e1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question: what happens when the chain is not optimism, can we have a guard and stop early?
services/wallet/router.go
Outdated
} | ||
|
||
return true, amountIn, estimate, bridgeAddress, nil | ||
l1Fee, _ := r.getL1Fee(ctx, network, token.Address, account, amountIn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Require approval is not a eth transaction, here we are calling get L1 fee assuming it is, isn't possible to call getL1Fees witht the data above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alaibe if we call it on other than the optimism chain we will get an error like L1 fee not supported on this chain and the value will be 0, so we continue the block execution, doesn't need to skip the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for answering and good for me, but this comment is about this specific call which need to buil getL1Fee for require approval not for eth tx
}, | ||
} | ||
|
||
tx, err := s.BuildTransaction(sendArgs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: all the belows seems repeated accross all bridge, maybe move this into fees.go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, thanks
|
||
l1GasFeeWei, _ := bridge.GetL1Fee(ctx, network, addrTo, addrFrom, token, amountIn) | ||
l1GasFeeWei += l1ApprovalFee | ||
gasFees.L1GasFee = weiToGwei(big.NewInt(int64(l1GasFeeWei))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be build directly instead of added?
I am not sure how @endulab has been doing it for communities but I believe they call directly:
status-go/services/wallet/api.go
Lines 396 to 399 in 590e352
func (api *API) GetSuggestedFees(ctx context.Context, chainID uint64) (*SuggestedFees, error) { | |
log.Debug("call to GetSuggestedFees") | |
return api.s.feesManager.suggestedFees(ctx, chainID) | |
} |
if this is the case he will need to update this api endpoint but also consider this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I use this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be build directly instead of added?
@alaibe if we have the following params network, addrTo, addrFrom, token, amountIn
on the client side, we can add a new endpoint and call it directly from the client, if that's what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sync up with @endulab offline, will create a follow up issue
6e30e1c
to
46955b1
Compare
46955b1
to
daed981
Compare
daed981
to
1c59e85
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for addressing this!
No description provided.