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 #1389

Conversation

kfastov
Copy link
Contributor

@kfastov kfastov commented Jan 22, 2024

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

  • Feature

What is the current behavior?

Genesis config builder does not check the existence of contract classes

Resolves: #1345

What is the new behavior?

  • The executable panics if a class hash mentioned in contracts or sierra_class_hash_to_casm_class_hash is not defined in contract_classes.

Does this introduce a breaking change?

No

Copy link
Collaborator

@tdelabro tdelabro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

@tdelabro
Copy link
Collaborator

@kfastov can you add a line in the CHANGELOG.md file please? So the CI can run ;)

@kfastov
Copy link
Contributor Author

kfastov commented Jan 23, 2024

@tdelabro Yes sure

@tdelabro
Copy link
Collaborator

@kfastov just fix the linters and we will merge that asap

@kfastov
Copy link
Contributor Author

kfastov commented Jan 24, 2024

Oh I see there is a linter issue in main now, fixed it in #1400

@kfastov kfastov changed the title feat: check that class exist before using it in BuildGenesisConfig dev: check that class exist before using it in BuildGenesisConfig Jan 24, 2024
@kfastov kfastov force-pushed the feat/build_genesis_config_check_class_hashes branch from a9aa0ca to e1348db Compare January 30, 2024 09:46
@tdelabro
Copy link
Collaborator

@kfastov Good job! I wait for the CI to pass and I will merge. Thanks a lot

@tdelabro tdelabro enabled auto-merge (squash) January 30, 2024 09:51
@tdelabro
Copy link
Collaborator

@kfastov tests are failing

@kfastov
Copy link
Contributor Author

kfastov commented Jan 31, 2024

@tdelabro I will check tests tomorrow and finish this PR finally. Also, I have done what you said (solution 2), but not yet finished the tests (finding a way to instantiate contract classes in test).

auto-merge was automatically disabled February 1, 2024 13:20

Head branch was pushed to by a user without write access

@kfastov
Copy link
Contributor Author

kfastov commented Feb 2, 2024

There is probably an issue with taplo, which had an update recently. I am checking out what the reason may be. We can pin it to v0.5.2 in CI script for now

@tdelabro
Copy link
Collaborator

tdelabro commented Feb 2, 2024

@kfastov you also have to fix a lil discrepancy between the error message expect and the actual one in rpc-tests

@kfastov kfastov requested a review from tdelabro February 11, 2024 03:56
@tdelabro tdelabro merged commit c7c70de into keep-starknet-strange:main Feb 12, 2024
12 checks passed
@tdelabro
Copy link
Collaborator

@kfastov thanks a lot for staying involved in this one although all the rebase conflicts

@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dev: check that class exist before using it in BuildGenesisConfig
4 participants