Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(starknet_mempool): rename to tx_from_address_exists to better reflect implementaion #3173

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

ArniStarkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@ArniStarkware ArniStarkware marked this pull request as ready for review January 8, 2025 09:58
Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @ayeletstarkware and @Yael-Starkware)


a discussion (no related file):
+reviewer:@Yael-Starkware
+reviewer:@ayeletstarkware

@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/mempool/rename_entrypoint branch 3 times, most recently from 9e1dfdb to 1e2c1af Compare January 8, 2025 14:03
Copy link
Contributor

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @Yael-Starkware)


crates/starknet_mempool/src/communication.rs line 105 at r2 (raw file):

    pub(crate) fn has_tx_from_address(
        &self,
        _contract_address: ContractAddress,

why?

Code quote:

contract_address

@ArniStarkware
Copy link
Contributor Author

crates/starknet_mempool/src/communication.rs line 105 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

why?

now account_address.
The name "account address" better reflects that it is a "sender_address" of some kind.

@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/mempool/rename_entrypoint branch from 1e2c1af to 815979b Compare January 8, 2025 15:23
Copy link
Contributor

@Yael-Starkware Yael-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

@ArniStarkware ArniStarkware requested a review from alonh5 January 9, 2025 06:33
@ArniStarkware
Copy link
Contributor Author

Previously, ArniStarkware (Arnon Hod) wrote…

+reviewer:@Yael-Starkware
+reviewer:@ayeletstarkware

+reviewer:@alonh5

@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/mempool/rename_entrypoint branch 2 times, most recently from 50044e1 to c3f2719 Compare January 9, 2025 10:11
Copy link
Contributor

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alonh5)

@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/mempool/rename_entrypoint branch from c3f2719 to d10f836 Compare January 9, 2025 14:24
@ArniStarkware ArniStarkware added this pull request to the merge queue Jan 9, 2025
Merged via the queue into main with commit 9523120 Jan 9, 2025
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 2025
Copy link
Collaborator

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions


crates/starknet_mempool_types/src/communication.rs line 49 at r3 (raw file):

    async fn commit_block(&self, args: CommitBlockArgs) -> MempoolClientResult<()>;
    async fn get_txs(&self, n_txs: usize) -> MempoolClientResult<Vec<AccountTransaction>>;
    async fn has_tx_from_address(

The signature of the function includes the arguments, so sometimes, we can avoid redundancy.

Suggestion:

contains_tx_from

crates/starknet_mempool_types/src/communication.rs line 51 at r3 (raw file):

    async fn has_tx_from_address(
        &self,
        contract_address: ContractAddress,

It's still an account address though, no?

Suggestion:

account_address

crates/starknet_mempool_types/src/communication.rs line 111 at r3 (raw file):

        contract_address: ContractAddress,
    ) -> MempoolClientResult<bool> {
        let request = MempoolRequest::DeployAccountExists(contract_address);

Match everywhere to new naming.

Code quote:

DeployAccountExists

@ArniStarkware
Copy link
Contributor Author

crates/starknet_mempool_types/src/communication.rs line 49 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

The signature of the function includes the arguments, so sometimes, we can avoid redundancy.

Done. See #3298.

@ArniStarkware
Copy link
Contributor Author

crates/starknet_mempool_types/src/communication.rs line 51 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

It's still an account address though, no?

Done. See #3298.

@ArniStarkware
Copy link
Contributor Author

crates/starknet_mempool_types/src/communication.rs line 111 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Match everywhere to new naming.

Done. See #3298.

@ArniStarkware
Copy link
Contributor Author

crates/starknet_mempool_types/src/communication.rs line 111 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done. See #3298.

ctrl+fed for:
has_tx_from_address
deploy_account_exists
DeployAccountExists
HasTx
HasTransaction

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.

None yet

5 participants