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_api): move transaction constants to snapi #2104

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

ArniStarkware
Copy link
Contributor

@ArniStarkware ArniStarkware commented Nov 17, 2024

In this PR we move the file transaction/constants.rs from blockifier to starknet_api (and only that).

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware force-pushed the arni/snapi/share_transaction_constants branch from d4c55e7 to 8fa9411 Compare November 17, 2024 11:42
Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware force-pushed the arni/snapi/share_transaction_constants branch from 8fa9411 to fa4cc11 Compare November 17, 2024 11:54
Copy link

Artifacts upload triggered. View details here

Copy link

codecov bot commented Nov 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.34%. Comparing base (e3165c4) to head (fa4cc11).
Report is 455 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2104       +/-   ##
===========================================
+ Coverage   40.10%   77.34%   +37.23%     
===========================================
  Files          26      105       +79     
  Lines        1895    13818    +11923     
  Branches     1895    13818    +11923     
===========================================
+ Hits          760    10687     +9927     
- Misses       1100     2672     +1572     
- Partials       35      459      +424     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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 15 files reviewed, all discussions resolved (waiting on @ayeletstarkware and @MohammadNassar1)


crates/starknet_api/src/transaction/constants.rs line 8 at r1 (raw file):

pub const DEPLOY_CONTRACT_FUNCTION_ENTRY_POINT_NAME: &str = "deploy_contract";

pub const TRANSFER_EVENT_NAME: &str = "Transfer";

We need at least EXECUTE_ENTRY_POINT_NAME in #2106, but all of these entry-point names sound just as useful.

Code quote:

pub const EXECUTE_ENTRY_POINT_NAME: &str = "__execute__";
pub const TRANSFER_ENTRY_POINT_NAME: &str = "transfer";
pub const VALIDATE_ENTRY_POINT_NAME: &str = "__validate__";
pub const VALIDATE_DECLARE_ENTRY_POINT_NAME: &str = "__validate_declare__";
pub const VALIDATE_DEPLOY_ENTRY_POINT_NAME: &str = "__validate_deploy__";
pub const DEPLOY_CONTRACT_FUNCTION_ENTRY_POINT_NAME: &str = "deploy_contract";

pub const TRANSFER_EVENT_NAME: &str = "Transfer";

crates/starknet_api/src/transaction/constants.rs line 12 at r1 (raw file):

// Cairo constants.
pub const FELT_FALSE: u64 = 0;
pub const FELT_TRUE: u64 = 1;

These are only used in tests for now. Seems generally useful (Maybe we should even make it a property of Felt?)

Code quote:

// Cairo constants.
pub const FELT_FALSE: u64 = 0;
pub const FELT_TRUE: u64 = 1;

crates/starknet_api/src/transaction/constants.rs line 15 at r1 (raw file):

// Expected return value of a `validate` entry point: `VALID`.
pub const VALIDATE_RETDATA: &str = "0x56414c4944";

Seems generally useful.

Code quote:

// Expected return value of a `validate` entry point: `VALID`.
pub const VALIDATE_RETDATA: &str = "0x56414c4944";

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.

Are these constants going to be used outside of blockifier?

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


crates/blockifier/src/abi/abi_utils_test.rs line 6 at r1 (raw file):

use crate::abi::abi_utils::selector_from_name;
use crate::abi::constants as abi_constants;

Is this going to be used outside of blockifier? perhaps this should be moved as well

Code quote:

abi_constants

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.

Yes, at least the first constant in #2106.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MohammadNassar1)


crates/blockifier/src/abi/abi_utils_test.rs line 6 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Is this going to be used outside of blockifier? perhaps this should be moved as well

Yes! This is what #2105 is all about.


crates/starknet_api/src/transaction/constants.rs line 8 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

We need at least EXECUTE_ENTRY_POINT_NAME in #2106, but all of these entry-point names sound just as useful.

@ayeletstarkware, you asked if these constants would be used outside of the blockifier crate.
Yes, at least the first constant in #2106.

Copy link
Contributor

@MohammadNassar1 MohammadNassar1 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

Copy link
Contributor Author

ArniStarkware commented Nov 18, 2024

Merge activity

  • Nov 18, 4:38 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Nov 18, 4:39 AM EST: A user merged this pull request with Graphite.

@ArniStarkware ArniStarkware merged commit 62dbf7a into main Nov 18, 2024
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 20, 2024
@ArniStarkware ArniStarkware deleted the arni/snapi/share_transaction_constants branch November 28, 2024 13:46
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.

4 participants