Fix state-caching #916
Fix state-caching #916
Conversation
richardpringle
force-pushed
the
fix-state
branch
2 times, most recently
from
ec25ebc
to
4bb14da
Compare
| Fields::Named(_) => quote! { | ||
| Self::#variant_ident { .. } => panic!("named enum fields are not supported") |
There was a problem hiding this comment.
This could be turned into a compile-time error instead of a runtime one for more safety
There was a problem hiding this comment.
Woooooow, I was just moving this, I didn't change it. You're absolutely correct, good catch!
| context_type.unwrap().arguments = segment.arguments.clone(); | ||
| } | ||
|
|
||
| type_path.path.segments.last() == context_type.segments.last() | ||
| } else { | ||
| false | ||
| } |
There was a problem hiding this comment.
What do you think about writing a few unit tests here ?
| } | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use crate::is_context; | |
| #[test] | |
| fn not_a_context() { | |
| assert!(!is_context(&Box::new( | |
| syn::parse_str::<syn::Type>("std::u8").unwrap() | |
| ))); | |
| } | |
| #[test] | |
| fn context_short() { | |
| assert!(is_context(&Box::new( | |
| syn::parse_str::<syn::Type>("Context").unwrap() | |
| ))); | |
| } | |
| #[test] | |
| fn context_full() { | |
| assert!(is_context(&Box::new( | |
| syn::parse_str::<syn::Type>("wasmlanche_sdk::Context").unwrap() | |
| ))); | |
| } | |
| } |
There was a problem hiding this comment.
I would approve a PR with this haha. I don't quite think it's necessary right now because the ui tests cover the cases pretty well, but more unit tests usually make things easier to expand and extend.
| { | ||
| cache: HashMap<K, Vec<u8>>, | ||
| pub struct State<'a, K: Key> { | ||
| cache: &'a RefCell<HashMap<K, Vec<u8>>>, |
There was a problem hiding this comment.
Should we be a bit more restrictive about https://github.com/ava-labs/hypersdk/blob/main/x/programs/v2/config.go#L161 because our cache which is now a RefCell is not thread safe ?
There was a problem hiding this comment.
If programs were multi-threaded, I think it would be a lot harder to guarantee determinism. I don't think it's something we're going to have to worry about in the near future.
Also, RefCell isn't Sync, you can still send it across threads
https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#impl-Send-for-RefCell%3CT%3E
richardpringle
force-pushed
the
fix-state
branch
2 times, most recently
from
e8405d8
to
e7fc876
Compare
richardpringle
force-pushed
the
fix-state
branch
4 times, most recently
from
6e9a40f
to
1740f35
Compare
|
Is splitting the functions into "func" and "func_internal" needed? It seems a little messy, so it would be nice if that weren't necessary. |
TL;DR Response: I agree it's a little messy for now. I want to go in and generate the code for program-to-program calls (#723) then and maybe get #595 in as well, then take a look at the API and calling conventions and see what needs cleaning up. I have a couple of different ideas on how to deal with that |
| } | ||
|
|
||
| fn mint_to_internal( | ||
| context: Context<StateKeys>, |
There was a problem hiding this comment.
I think that it would make more sense to pass the context by reference to internal functions
There was a problem hiding this comment.
Yeah, I agree. This isn't going to stay like this though.
See the comment above
Before the state-cache was created every time you called `Program::state`, but that means that it's not persisted for the lifetime of `Program` (and therefore we don't actually get any cacheing behaviour). I've fixed the lifetime issues, but this design still needs work.
State-caching appears to have always been a little broken. While fixing this, I also realized that the
state_keymacro was doing a bunch of things that didn't make any sense, so I'm also fixing that since I have to change it anyway.