fix: make subnet nodes configurable and fix https outcall cost calculation #351
Conversation
ninegua
force-pushed
the
paulliu/fix-http-request-cost-calculation
branch
from
1259708 to
b4d4885
Compare
ninegua
force-pushed
the
paulliu/fix-http-request-cost-calculation
branch
from
b4d4885 to
e98a9d3
Compare
ninegua
force-pushed
the
paulliu/fix-http-request-cost-calculation
branch
from
f0f049d to
dc90e48
Compare
| @@ -108,6 +108,7 @@ type InstallArgs = record { | |||
| manageApiKeys : opt vec principal; | |||
| logFilter : opt LogFilter; | |||
| overrideProvider : opt OverrideProvider; | |||
| nodesInSubnet : opt nat32; | |||
There was a problem hiding this comment.
| nodesInSubnet : opt nat32; | |
| numNodesInSubnet : opt nat32; |
There was a problem hiding this comment.
There is already a method called getNodesInSubnet, so I'm reusing the same term nodesInSubnet to be consistent in the interface.
| let nodes_in_subnet = NODES_IN_SUBNET as u128; | ||
| /// See https://internetcomputer.org/docs/current/developer-docs/gas-cost/#https-outcalls | ||
| pub fn get_http_request_cost( | ||
| nodes_in_subnet: u32, |
There was a problem hiding this comment.
nit: I would use num_nodes_in_subnet here and everywhere to be more explicit.
| nodes_in_subnet: u32, | |
| num_nodes_in_subnet: u32, |
| @@ -1,15 +1,5 @@ | |||
| // HTTP outcall cost calculation | |||
| // See https://internetcomputer.org/docs/current/developer-docs/gas-cost#special-features | |||
| // Constant used in HTTP outcall cost calculation | |||
| pub const INGRESS_OVERHEAD_BYTES: u128 = 100; | |||
There was a problem hiding this comment.
Is there perhaps a link where this is documented? I'm not super familiar with all of this so I appreciate when there is documentation available :)
There was a problem hiding this comment.
I have no idea why this gets the value 100 in the first place. But I guess it was some heuristics?
| @@ -1042,6 +1049,38 @@ fn candid_rpc_should_err_without_cycles() { | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn candid_rpc_should_err_with_insufficient_cycles() { | |||
There was a problem hiding this comment.
nit: I would add a test for when the number of nodes in the subnet is not specified in the install args.
XC-257
Fix the cycle computation according to the latest document on the cost for https outcalls https://internetcomputer.org/docs/current/developer-docs/gas-cost/#https-outcalls
Also add
nodesInSubnetas an optional initialization parameter when creating the EVM RPC canister. It defaults to 34 if not set, which is also the default used previously.