replace concrete blockstore in TestVM with a generic trait #1259
Conversation
Merge queue setting changed
There was a problem hiding this comment.
Thanks.
FYI see discussion in #133 and filecoin-project/ref-fvm#1694 about whether the blockstore should instead by a dyn reference rather than generic parameter, and possible changes to its API. I think that because it's currently not object-safe that it's not an option for this PR for the TestVM to hold a blockstore reference, but if it were possible or becomes so, I think that would be better.
FYI @Stebalien
| where | ||
| BS: Blockstore, | ||
| { | ||
| pub store: &'bs BS, | ||
| pub state_root: RefCell<Cid>, |
There was a problem hiding this comment.
In a follow-up, these should probably both become non-pub if they're to be abstracted over by trait.
| where | ||
| BS: Blockstore, | ||
| { | ||
| pub fn new(store: &'bs MemoryBlockstore) -> TestVM<'bs, MemoryBlockstore> { |
There was a problem hiding this comment.
Is it possible/easy to make this store parameter a BS while you're here?
Yeh that was the motivation to make this change here. I would have preferred the ability to use something like filecoin-project/ref-fvm#1694 and a dyn Blockstore upon initial consideration. |
Further groundwork for an abstract VM trait. Especially in the context of benchmarking it may be useful to inject TrackingBlockstores etc. in future.
The current TestVM could not implement a method that returns a BS: Blockstore which would be required by a generic VM<BS: Blockstore> trait.