feat: add canister init args #112
Conversation
| @@ -9,6 +9,7 @@ edition = "2021" | |||
| [profile.release] | |||
| debug = false | |||
| lto = true | |||
| strip = true | |||
There was a problem hiding this comment.
(Quick fix for getting below the 2 MB Wasm size limit)
rvanasa
changed the title
| (EvmRpcCanister, "default", 13, 521_498_000), | ||
| (EvmRpcFidicuaryCanister, "fiduciary", 28, 1_123_226_461), |
There was a problem hiding this comment.
It is not clear what these numbers mean (readers may be able to guess what 13 and 28 mean but not the other numbers).
How about either adding a comment or replacing the plain numbers with meaningfully named constants?
There was a problem hiding this comment.
Added a comment to clarify. There's also a line directly after this which shows the variable names for each tuple entry, although I see how it could be easy to miss.
|
|
||
| let candidRpcCycles = 1_000_000_000_000; |
There was a problem hiding this comment.
Why is there so much code in red and identical-looking code in green below..?
There was a problem hiding this comment.
This is due to a change in indentation (modified the E2E test to cover both the standard and fiduciary canisters).
src/accounting.rs
Outdated
| @@ -35,7 +35,7 @@ pub fn get_http_request_cost( | |||
| json_rpc_payload: &str, | |||
| max_response_bytes: u64, | |||
| ) -> u128 { | |||
| let nodes_in_subnet = METADATA.with(|m| m.borrow().get().nodes_in_subnet); | |||
| let nodes_in_subnet = TRANSIENT_SUBNET_SIZE.with(|m| *m.borrow()); | |||
There was a problem hiding this comment.
What's a "transient subnet size"?
There was a problem hiding this comment.
Data stored in non-stable memory uses a TRANSIENT_ prefix. Open to suggestions for a different naming convention (also see response below).
src/memory.rs
Outdated
| @@ -21,6 +21,7 @@ declare_log_buffer!(name = ERROR, capacity = 1000); | |||
| thread_local! { | |||
| // Transient static data: this is reset when the canister is upgraded. | |||
| pub static TRANSIENT_METRICS: RefCell<Metrics> = RefCell::new(Metrics::default()); | |||
| pub static TRANSIENT_SUBNET_SIZE: RefCell<u32> = RefCell::new(DEFAULT_NODES_IN_SUBNET); | |||
There was a problem hiding this comment.
So, it's called "transient" because this information is not persisted?
It's a bit misleading in my opinion because an upgrade doesn't change the subnet size.
There was a problem hiding this comment.
Do you know if there is a conventional name for non-stable memory on the IC? I've seen "main memory" and "flexible memory" but neither seem to work better in this case. I suppose we could remove the TRANSIENT_ prefix and define these constants in stable::* and unstable::* modules, or something along those lines.
For now I'll rename these to UNSTABLE_* as a slightly clearer alternative.
There was a problem hiding this comment.
I like the approach with different modules better. "Unstable subnet size" also sounds weird. 🙂
But we can go with your suggestion for now and have some bike-shedding fun later.
Replaces
getNodesInSubnet()andsetNodesInSubnet()methods with a Candid init argument:The ideal situation would be to specify defaults in the
dfx.jsonfile, but this doesn't seem to be possible at the moment.