Issue #1502 add a name field in GenesisClassJson (Unfinished) #1552
Conversation
There was a problem hiding this comment.
First of all, thanks for the contribution!
I believe we can do this with a simpler approach. During this phase, where we compute the class hashes of all the classes, at the same time we can also build up a mapping of class name to its respective class hash. This mapping can later then be used throughout the method for resolving any class names to its class hash.
|
We also need to modify all instances of this class field (basically here, here, and here) so that it can hold either a class name or the class hash, and perform the resolution accordingly. |
|
@Yogalholic how are you feeling about this? Do you need any help / additional information? |
|
As you can see in me last PR, I am struggling with adding new GenesisJsonError and how to handle FieldElement. |
What trouble are you having? Can you point out where in the code that you have trouble with? |
Firstly, could you check please if I'm on the right direction ? Is creating an enum ClassOrHash was a good idea? |
|
Any feedback please? |
| // resolves the class paths, if any | ||
| genesis.resolve_class_artifacts(path)?; | ||
| genesis.resolve_class_artifacts(path, &class_name_map)?; |
There was a problem hiding this comment.
| genesis.resolve_class_artifacts(path, &class_name_map)?; | |
| genesis.resolve_class_artifacts(path)?; |
This functions resolves the artifacts, which consist on loading the class JSON into memory to be available for genesis operation. The name here shouldn't be relevant as we are expecting to use the names into other entries, not the classes themselves.
|
Hey @Yogalholic, do you need any support or info to tackle this? Don't hesitate to reach out if yes. 👍 |
|
In my last PR I have implemented your suggestions. I still don't know how to add a new GenesisJsonError |
This one you mean? |
|
Any feedback please? I have run the rust_fmt script |
There was a problem hiding this comment.
Thanks for the progress here @Yogalholic!
I've left some comment. Also, if you have more doubt don't hesitate to reach out on the discord we would be happy to help!
You can also start to add the name for testing once you're confident with the changes. 👍
| Ok((class_hash, GenesisClass { compiled_class_hash, sierra, casm })) | ||
| }) | ||
| .collect::<Result<_, GenesisJsonError>>()?; | ||
|
|
||
| // Populate the classes and name_to_class_hash |
There was a problem hiding this comment.
| // Populate the classes and name_to_class_hash |
| Some(NameOrHash::ClassHash(class_hash)) => class_hash, | ||
| Some(NameOrHash::ClassName(class_name)) => *name_to_class_hash | ||
| .get(&class_name) | ||
| .ok_or_else(|| value_out_of_range_error::ValueOutOfRangeError)?, |
There was a problem hiding this comment.
Here we want to return a new error something like:
#[error("Class name not found in the genesis classes: {0}")]
InvalidClassName(String)You should add this to the GenesisJsonError.
This applies to all conversions from name that fails.
| } | ||
| }; | ||
|
|
||
| // Populate the classes and name_to_class_hash |
There was a problem hiding this comment.
Why do we have this code two times? Did you mean to replace it?
| // Deserialize JSON | ||
| let mut genesis_json: GenesisJson = serde_json::from_slice(&decoded)?; | ||
|
|
||
| // Populate name field |
There was a problem hiding this comment.
What do you want to achieve here? The name of the class is supposed to be given by the user into the genesis json file. :) We don't need to retrieve it from the artifact file itself.
There was a problem hiding this comment.
OK. I have removed the for loop
| (class_hash, class_hash, None, Arc::new(CompiledContractClass::V0(casm))) | ||
| } | ||
| }; | ||
|
|
||
| // Add the class name and class hash to the mapping | ||
| let _ = name.map(|name| { |
There was a problem hiding this comment.
We don't want the user to define 2 classes with the same name. Could we add a check to ensure that the names are unique for only ONE class?
Thanks for that. You could also run the |
|
I have implemented every suggestion, the code is compiling fine. I updated the first unit test (deserialize_from_json() ) and modified the test-genesis.json to replace the "0x08" classhash with the random name "MyErc20". The test is successful and now I will update the rest of them. |
Awesome, thanks for the heads up @Yogalholic and the work done here. A recent work was merged on Katana for the allocations to compute the total supply automatically for the fee token. You have a rebase here. Don't hesitate to ping us if you have any question/difficulty while merging. |
|
@Yogalholic don't forget to rebase on main to ensure tests can be run on the CI. 👍 |
|
I have finished working on the issue, the code compiles and tests are successful but clippy is failing |
resolves #1502
resolves DOJ-130