0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
612 stars 150 forks source link

Fixed imports serialization #1303

Closed polydez closed 3 months ago

polydez commented 3 months ago

I've faced issue with imports serialization. We don't serialize import names due to the fact that name equals to the latest identifier in the path. Sometimes this rule doesn't work. For example, if we create new TransactionKernel::assembler() we can find this different name in its imports:

"wallet": LibraryPath { path: "miden::contracts::wallets::basic", num_components: 4 }

Thus, sometimes name differs from the last path element. This PR should fix this issue by optionally serializing name of import (if it differs).

hackaugusto commented 3 months ago

I'm unfamiliar with this bit of code, and can't really tell how it should behave. Can you give a concrete example of what is this fixing? Perhaps a unit test?

polydez commented 3 months ago

I'm unfamiliar with this bit of code, and can't really tell how it should behave. Can you give a concrete example of what is this fixing? Perhaps a unit test?

When I tried to instantiate TransactionKernel::assembler() and then serialize and deserialize it, I've faced that deserialized assembler doesn't equal to initial one in part of its imports. This is because, according to comment in the current code, author assumes that import name always equals the last segment of import's path and current solution doesn't serialize name of the import at all, calculating it on deserialization. But import of basic wallet has path "miden::contracts::wallets::basic" and name "wallet" which, obviously, can't be figured out from the path.

This fixes incorrect (de)serialization of assembly imports by serializing imports name when it can't be calculated automatically.

hackaugusto commented 3 months ago

so, if I understood correctly this happens when we serialize a reexport, e.g. use.miden::contracts::wallets::basic->wallet? I still think this needs a unit test. And it should probably be done after https://github.com/0xPolygonMiden/miden-vm/pull/1277

polydez commented 3 months ago

so, if I understood correctly this happens when we serialize a reexport, e.g. use.miden::contracts::wallets::basic->wallet?

Yes, it seems so. Thank you, I will add a unit test for this.

polydez commented 3 months ago

I've found that code:

use.miden::contracts::wallets::basic->wallet

Thus, it renames an import. Will create a unit-test for this.

polydez commented 3 months ago

@hackaugusto, I've added a unit-test.

bitwalker commented 3 months ago

@polydez I suspect this won't be relevant anymore after #1277 is merged, since the way imports are represented has changed (the ModuleImports type doesn't exist any more for example). I think the issue you hit should be resolved as a result of the changes that were implemented in that PR, even if the representation of things has been reworked.

I know we're hoping to get that merged by next week, but if this issue is blocking you until that happens, it's not a big deal if this gets merged in the meantime.

polydez commented 3 months ago

Thank @bitwalker, I will wait for merging of solution you mentioned.