feat: implement AccountComponentBuilder#1734
feat: implement AccountComponentBuilder#1734PhilippGackstatter wants to merge 2 commits intonextfrom
AccountComponentBuilder#1734Conversation
|
This looks good, I think it would match the overall style of following the builder pattern across miden-base. Also one of the pioneers was facing issues with the |
| #[derive(Clone)] | ||
| pub struct AccountComponentBuilder { | ||
| assembler: Assembler, | ||
| storage_slots: Vec<StorageSlot>, | ||
| supported_types: BTreeSet<AccountType>, | ||
| } |
There was a problem hiding this comment.
Thinking about it some more, I think this is fine if the goal is to only replace AccountComponent::compile. I'm not sure if this eliminates the need for users to use the TransactionKernel::assembler, though. Basically, I'm wondering how end users are creating account components.
The naive approach would be something like:
const COMPONENT_SOURCE_CODE: &str = "export.myprocedure nop end"
pub struct MyAccountComponent {
storage_slot: StorageSlot,
}
impl MyAccountComponent {
pub fn new(...) -> Result<Self, MyError> {
// Validate whatever needs to be validated.
Self { ... }
}
}
impl From<MyAccountComponent> for AccountComponent {
fn from(my_component: MyAccountComponent) -> Self {
AccountComponentBuilder::default()
.with_storage_slot(my_component.storage_slot)
.with_supported_type(AccountType::RegularAccountUpdatableCode)
.build(COMPONENT_SOURCE_CODE)
.expect("my account component should satisfy the requirements of a valid account component");
}
}That is, the source code is passed to the builder directly. But if I was a user, I would probably want to use a similar approach as we do for assembling the library statically in a LazyLock, e.g.:
static COMPONENT_LIBRARY: LazyLock<Library> = LazyLock::new(|| {
let source = NamedSource::new("my::account", COMPONENT_SOURCE_CODE);
TransactionKernel::assembler()
.assemble_library([source])
.expect("my account code should be valid")
});
impl From<MyAccountComponent> for AccountComponent {
fn from(my_component: MyAccountComponent) -> Self {
AccountComponentBuilder::default()
.with_storage_slot(my_component.storage_slot)
.with_supported_type(AccountType::RegularAccountUpdatableCode)
.build(COMPONENT_LIBRARY.clone())
.expect("my account component should satisfy the requirements of a valid account component");
}
}That way, the library is only assembled once and this approach also supports building the components in a build.rs script like we do, which is better to catch assembly errors at build time.
Note how this still uses an Assembler directly. So if we want to support this last approach, I think we may need something that builds account component Librarys. We can have this be a separate type altogether, but its API would be very similar to the ScriptBuilder and so this sounds like an opportunity to deduplicate code. That functionality could be implemented by adding a ScriptBuilder::compile_library. The thing is that this would expand the scope of the ScriptBuilder beyond scripts and make it a bit more like the general-purpose Assembler, so I think we'd need a new name for it. Maybe a generic CodeBuilder or ProtocolAssembler to reflect the fact that it's basically a tailor-made Assembler for the Miden protocol.
With that, it could still make sense to have an AccountComponentBuilder, but instead of assembling code, it would just take a Library. Might make constructing AccountComponents a bit more convenient, but that's then a somewhat separate discussion. Any thoughts @partylikeits1983, @mmagician?
There was a problem hiding this comment.
by adding a ScriptBuilder::compile_library. The thing is that this would expand the scope of the ScriptBuilder beyond scripts and make it a bit more like the general-purpose Assembler, so I think we'd need a new name for it. Maybe a generic CodeBuilder or ProtocolAssembler to reflect the fact that it's basically a tailor-made Assembler for the Miden protocol.
I think this is a good idea since it abstracts away the assembler. The most important part, is that the account component builder should be able to take a library path param, i.e. a NamedSource. Adding compile_library to the ScriptBuilder is a great idea. I wrote similar functionality (not in a builder pattern) in the miden-client-tools repo.
A good name might be CodeBuilder, ProtocolAssembler, or ModuleBuilder
There was a problem hiding this comment.
I guess the main difference between AccountComponentBuilder and ScriptBuilder is that the latter is just for assembling script programs while the latter is going beyond that. That is ScriptBuilder does try to create a full Note object - but just the NoteScript.
So, it does seem like maybe the current AccountComponentBuilder is mixing two things together:
- Assembling the library for the component.
- Building the actual component.
Combining the two into a single struct maybe a bit difficult because we could build account components without the need to assemble the library (e.g., in cases when the library has been previously assembled and we just need to deserialize it).
We could create something like AccountComponentCode(Library) struct (and the corresponding AccountComponentCodeBuilder) but this may be an overkill.
At the same time, I'm not sure yet about creating a single struct that handles all assembly needs (it may be a good idea, but I haven't thought it through yet). Seems like additional benefit over exposing the original assembler is not big - but maybe that's fine. I guess we could even call this struct just Assembler and the main difference from miden-vm assembler would be that we pre-load kernel into it and have compile_tx_script(), compile_note_script(), and compile_component_code() methods on it?
There was a problem hiding this comment.
Generally agreed, I also wouldn't mix assembling and building components in the same type. (That's basically what claude code came up with when I told it to replace AccountComponent::compile with a ScriptBuilder-like interface).
Seems like additional benefit over exposing the original assembler is not big - but maybe that's fine. I guess we could even call this struct just
Assemblerand the main difference frommiden-vmassembler would be that we pre-load kernel into it and havecompile_tx_script(),compile_note_script(), andcompile_component_code()methods on it?
Yeah, agreed. I still think the ScriptBuilder is a good idea as it provides a minimal, but sufficiently powerful API for the assembly needs of the protocol, while the VM Assembler goes a bit beyond that (like having the ability to assemble kernels). So having compile_tx_script, compile_note_script, and compile_component_code does seem like a very nice API that should cover everything.
I think I like the AccountComponentCode wrapper so that compile_component_code returns that instead of the generic Library, which would seem a bit at odds with its name (otherwise it should be called compile_library - but I do like the more specific compile_component_code).
Regarding naming:
- I think I wouldn't call it
Assembler, just to avoid confusion with the VM one. We probably no longer would expose the VMAssemblerinmiden-objects, so the chance for confusion is minimized, but still not sure it's a good idea. TransactionAssemblerin theory fits because all scripts and libraries it assembles are assembled against the tx kernel, but the first association is that it assembles transactions. SoTxKernelAssembleris a bit better, but a bit verbose.ProtocolAssembleris not amazing, but the one I like most, since it's a bit more precise than the genericCodeBuilder.
There was a problem hiding this comment.
For the name, I probably would prefer TransactionAssembler over others (TxKernelAssembler sounds like it is for assembling transaction kernels, and similarly) - but also, don't really love it.
|
Opened #1756 to cover the part for assembling account component code. After that, we can still think about having an |
I've asked claude to implement an
AccountComponentBuilderto removeAccountComponent::compileand as a parallel toScriptBuilder, as an idea for fixing 0xMiden/miden-client#1146 (comment).Should we go ahead with something like this? I haven't reviewed the PR, but the approach seems good. If we want to go ahead, I'll clean up the PR and make it ready for a proper review.