Skip to content
This repository has been archived by the owner on Aug 2, 2024. It is now read-only.

dev: check that class exist before using it in BuildGenesisConfig #1345

Closed
tdelabro opened this issue Jan 4, 2024 · 6 comments · Fixed by #1389
Closed

dev: check that class exist before using it in BuildGenesisConfig #1345

tdelabro opened this issue Jan 4, 2024 · 6 comments · Fixed by #1389
Assignees
Labels
backlog Ready to be picked bug Something isn't working enhancement New feature or request feature New feature

Comments

@tdelabro
Copy link
Collaborator

tdelabro commented Jan 4, 2024

Problem

    #[pallet::genesis_build]
    impl<T: Config> BuildGenesisConfig for GenesisConfig<T> {
        fn build(&self) {
            <Pallet<T>>::store_block(0);
            frame_support::storage::unhashed::put::<StarknetStorageSchemaVersion>(
                PALLET_STARKNET_SCHEMA,
                &StarknetStorageSchemaVersion::V1,
            );

            for (class_hash, contract_class) in self.contract_classes.iter() {
                ContractClasses::<T>::insert(class_hash, contract_class);
            }

            for (sierra_class_hash, casm_class_hash) in self.sierra_to_casm_class_hash.iter() {
                CompiledClassHashes::<T>::insert(sierra_class_hash, CompiledClassHash(casm_class_hash.0));
            }

            for (address, class_hash) in self.contracts.iter() {
                ContractClassHashes::<T>::insert(address, class_hash);
            }

            for (key, value) in self.storage.iter() {
                StorageView::<T>::insert(key, value);
            }

            LastKnownEthBlock::<T>::set(None);
            // Set the fee token address from the genesis config.
            FeeTokenAddress::<T>::set(self.fee_token_address);
            SeqAddrUpdate::<T>::put(true);
        }
    }

Here we "declare" classes:

 for (class_hash, contract_class) in self.contract_classes.iter() {
        ContractClasses::<T>::insert(class_hash, contract_class);
}

Here we map them to their sierra

for (sierra_class_hash, casm_class_hash) in self.sierra_to_casm_class_hash.iter() {
       CompiledClassHashes::<T>::insert(sierra_class_hash, CompiledClassHash(casm_class_hash.0));
}

And here we "deploy" new contracts:

            for (address, class_hash) in self.contracts.iter() {
                ContractClassHashes::<T>::insert(address, class_hash);
            }

It would be nice for "declare", "map ot sierra" and "deploy" to make sure we are refering to ContractClass that actually exist. Meaning that have been stored in the first storage ContractClasses.

What to do?

Add those checks, panic if we are using a ClassHash which don't have been declared before in the first for loop

@tdelabro tdelabro added the enhancement New feature or request label Jan 4, 2024
@tdelabro tdelabro added this to Madara Jan 4, 2024
@tdelabro tdelabro added bug Something isn't working feature New feature backlog Ready to be picked labels Jan 4, 2024
@tdelabro tdelabro moved this to 🔖 Ready in Madara Jan 4, 2024
@kfastov
Copy link
Contributor

kfastov commented Jan 9, 2024

I would like to take this

@tdelabro
Copy link
Collaborator Author

tdelabro commented Jan 9, 2024

Rather than checking it is in the storage. You can check it is in the in-memory array. I will probably be a bit more efficient

@kfastov
Copy link
Contributor

kfastov commented Jan 10, 2024

I've implemented it using ContractClasses::<T>::contains_key(class_hash), I am a bit worried about O(n^2) complexity in the case of array, not sure how storage lookup is implemented though. I will test this simple implementation compared to an in-memory array and choose the best option

@tdelabro
Copy link
Collaborator Author

I think the storage lookup is a kind of hashmap. Due to caching. So probably better performances.
Forget what I said

@tdelabro
Copy link
Collaborator Author

Hey, @kfastov, do you have an ETA for opening a PR? understood you were quite close

@tdelabro tdelabro moved this from 🔖 Ready to 🏗 In progress in Madara Jan 19, 2024
@kfastov
Copy link
Contributor

kfastov commented Jan 20, 2024

Hey, sorry for delaying it so hard, will open PR tomorrow (Jan 21st)

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Madara Feb 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backlog Ready to be picked bug Something isn't working enhancement New feature or request feature New feature
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants