Remove duplicate procedures from miden-lib
#1002
Conversation
Fumuran
force-pushed
the
andrew-remove-duplicate-procedures
branch
2 times, most recently
from
1c545f6 to
787743f
Compare
Fumuran
force-pushed
the
andrew-remove-duplicate-procedures
branch
from
787743f to
b6267d9
Compare
|
This solution implements the whole new |
There was a problem hiding this comment.
Thank you! Looks good. I left a few comment inline, but basically, there are two properties I'd like to retain:
- Ideally, we won't need to add a new library to the list of libraries that we need to load into the executor and transaction kernel assembler.
- Also, we should try to avoid changing the public interface of the modules in
miden-lib(would should be able to just re-export procedures from the modules in which they were previously defined).
We could have achieved both of these if we had vendored library support in the assembler, but without it, I think we may need to rely on adding the MASM file with common utilities to tx kernel and miden lib libraries manually at build time.
| #! Output: [max_inputs_per_note] | ||
| #! | ||
| #! - max_inputs_per_note is the max inputs per note. | ||
| export.get_max_inputs_per_note |
There was a problem hiding this comment.
Is there a reason not to re-export the same procedure from here? For example, like:
export.utils::note::get_max_inputs_per_note
| #! Outputs: [fungible_asset_max_amount] | ||
| #! | ||
| #! fungible_asset_max_amount is the maximum amount of a fungible asset. | ||
| export.get_fungible_asset_max_amount |
There was a problem hiding this comment.
Same question as above: is there a reason we can't just re-export this procedure from utils?
| proc.type | ||
| u32split swap drop push.ACCOUNT_TYPE_U32MASK u32and | ||
| # => [acct_type] | ||
| end |
| #[derive(Clone)] | ||
| pub struct UtilsLib(Library); |
There was a problem hiding this comment.
Let's add a brief doc-comment explaining what this library contain.
| // load utils lib MAST forest | ||
| let utils_lib_forest = UtilsLib::default().mast_forest().clone(); | ||
| store.insert(utils_lib_forest); |
There was a problem hiding this comment.
I'm not sure I like that we now need to care about another library. Ideally, we'd just vendor the utils library into the miden library at compile time, but unfortunately, Miden assembler does not yet support vendoring. So, maybe trying the approach with a single MASM file is worth it.
cc @plafer, @bitwalker for another use case for vendored library.
This PR moves the duplicate
masmprocedures to the newutilslibrary.Closes: #836