refactor(transaction): make the only query field in declare tx public

[meship-starkware]

This change is 

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @noaov1)

a discussion (no related file):
@tdelabro

---

[noaov1]

Doing this allow to create an incorrect declare transaction (we want to enforce calling verify_contract_class_version).

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

[noaov1]

You can let the create method be public. We may need to consider the necessity of the method new_for_query (and maybe define new only for tests).

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @noaov1)

---

crates/blockifier/src/transaction/transactions.rs line 136 at r2 (raw file):

    pub tx_hash: TransactionHash,
    // Indicates the presence of the only_query bit in the version.
    only_query: bool,

Changing only query to a pub trait will allow users to create a Declare tx struct without using create. we want to enforce calling verify_contract_class_version so it is problematic.

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @noaov1)

---

crates/blockifier/src/transaction/transactions.rs line 136 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Changing only query to a pub trait will allow users to create a Declare tx struct without using create. we want to enforce calling verify_contract_class_version so it is problematic.

@tdelabro WDYT? Do you have another suggestion?

[tdelabro]

@meship-starkware
starkware-libs/blockifier#1725 (comment)

You then need to be coherent and make it private for the other transactions types too

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @noaov1)

---

crates/blockifier/src/transaction/transactions.rs line 292 at r2 (raw file):

    // Indicates the presence of the only_query bit in the version.
    pub only_query: bool,
}

This change means that we will have to create a Deploy Account tx using new and new for query.

[meship-starkware]

After some discussion, we decided that making it private is not a good solution because it is less readable, it is not clear why the only query is private there, and it is a breaking change. A solution that @noaov1 has suggested is making the only query public in declare but making the class info private. This is also a breaking change, but it makes more sense because the declare struct needs to be private because we need to check the class info in the constructor.

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @noaov1)

[tdelabro]

making it private is not a good solution because it is less readable

What about adding a getter? tx.is_query() is not less readable thattx.only_query

making the only query public in declare but making the class info private

@noaov1 has a good point that if we force users to use our create method in order to check for the class_info data, maybe we shouldn't allow users to change it's content afterward.
Note that the create method don't just use the class_info

    fn create(
        declare_tx: starknet_api::transaction::DeclareTransaction,
        tx_hash: TransactionHash,
        class_info: ClassInfo,
        only_query: bool,
    ) -> TransactionExecutionResult<Self> {
        let declare_version = declare_tx.version();
        verify_contract_class_version(&class_info.contract_class(), declare_version)?;
        Ok(Self { tx: declare_tx, tx_hash, class_info, only_query })
    }

It uses data from both declare_tx and class_info to call verify_contract_class_version, so those two fields should be private and only accessible through getters

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @noaov1)

[meship-starkware]

It will be solved in a future PR.

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @noaov1)