-
Notifications
You must be signed in to change notification settings - Fork 13
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
fix: set gas limit and price for fill estimation #680
Conversation
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.
Is this PR ready to be merged or is this a WiP? @pxrl
It is ready for review |
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.
Would this be giving the user of the sdk too much flexibility?
Hmm, can you elaborate what you mean with too much flexibility? But I think we could break this PR up into two parts:
I think 1. should definitely get in. With 2. I am indifferent but I don't see any risk tbh. Especially if it is allowed at RPC call level. |
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.
But I think we could break this PR up into two parts:
- Allowing the gasPrice override - this is actually also a possible performance boost. This allows us to cache and use prefetched gas prices which will save some RPC calls.
- Allowing the gasLimit override
I think 1. should definitely get in. With 2. I am indifferent but I don't see any risk tbh. Especially if it is allowed at RPC call level.
Agree; I think #1 is a good addition. The use case for #2 has evaporated a bit since the underlying issue w/ the integrator was resolved by a small tweak in their contract, so I guess we can live without it, but we may find a use case for it in future.
Some integrators flagged an issue for fee estimation when setting custom messages. After some digging, a workaround is to set a large enough hardcoded gas limit and a gas price when calling
eth_estimateGas
. E.g. the followingeth_estimateGas
call fails wheregas
andgasPrice
are not set:curl https://arb1.arbitrum.io/rpc \ -X POST \ -H "Content-Type: application/json" \ --data '{"method":"eth_estimateGas","params":[{"from":"0x428ab2ba90eba0a4be7af34c9ac451ab061ac010","to":"0xe35e9842fceaca96570b734083f4a58e8f7c5f2a","data":"0x2e3781150000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000a4b1000000000000000000000000ccb0be73bc7386db701006762dd7f8638d176d48000000000000000000000000ccb0be73bc7386db701006762dd7f8638d176d4800000000000000000000000000000000000000000000000000000000000000000000000000000000000000003c499c542cef5e3811e1192ce70d8cc03d5c3359000000000000000000000000af88d065e77c8cc2239327c5edb3a432268e58310000000000000000000000000000000000000000000000000000000005f5e1000000000000000000000000000000000000000000000000000000000005f5e100000000000000000000000000000000000000000000000000000000000000008900000000000000000000000000000000000000000000000000000000ffffffff00000000000000000000000000000000000000000000000000000000ffffffff0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000018000000000000000000000000000000000000000000000000000000000000000400000000000000000000000005f851f67d24419982ecd7b7765defd64fbb50a97000000000000000000000000eefef4a7992d93a3d6f1135cc8ea02cd5b863c2a"}],"id":1,"jsonrpc":"2.0"}'
This one on the other hand succeeds with limit and price set
curl https://arb1.arbitrum.io/rpc \ -X POST \ -H "Content-Type: application/json" \ --data '{"method":"eth_estimateGas","params":[{"from":"0x428ab2ba90eba0a4be7af34c9ac451ab061ac010","to":"0xe35e9842fceaca96570b734083f4a58e8f7c5f2a","gas":"0x989680","gasPrice":"0x989680","data":"0x2e3781150000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000a4b1000000000000000000000000ccb0be73bc7386db701006762dd7f8638d176d48000000000000000000000000ccb0be73bc7386db701006762dd7f8638d176d4800000000000000000000000000000000000000000000000000000000000000000000000000000000000000003c499c542cef5e3811e1192ce70d8cc03d5c3359000000000000000000000000af88d065e77c8cc2239327c5edb3a432268e58310000000000000000000000000000000000000000000000000000000005f5e1000000000000000000000000000000000000000000000000000000000005f5e100000000000000000000000000000000000000000000000000000000000000008900000000000000000000000000000000000000000000000000000000ffffffff00000000000000000000000000000000000000000000000000000000ffffffff0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000018000000000000000000000000000000000000000000000000000000000000000400000000000000000000000005f851f67d24419982ecd7b7765defd64fbb50a97000000000000000000000000eefef4a7992d93a3d6f1135cc8ea02cd5b863c2a"}],"id":1,"jsonrpc":"2.0"}'
This PR therefore proposes a change to allow us to pass a custom gas limit and price into the fee calculator.