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

feat(starknet_api): derive serialize for executable transaction #426

Conversation

MohammadNassar1
Copy link
Contributor

@MohammadNassar1 MohammadNassar1 commented Aug 13, 2024

This change is Reviewable

Copy link
Contributor

@giladchase giladchase 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 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware and @elintul)

Copy link
Contributor

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

BTW i think you can rebase this onto main and insert directly.
Yr Prs using it will temporarily fail CI, not too bad since this can be merged quickly

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

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/executable-transaction/derive-serialize-and-deserialize branch from 2d8a077 to b14a78b Compare August 15, 2024 07:48
@MohammadNassar1 MohammadNassar1 changed the base branch from mohammad/mempool/pool-stores-internal-transaction to main August 15, 2024 07:48
Copy link
Contributor Author

@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.

Done

Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @ayeletstarkware and @elintul)

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/executable-transaction/derive-serialize-and-deserialize branch from b14a78b to 5769af0 Compare August 15, 2024 07:53
@MohammadNassar1 MohammadNassar1 changed the title feat(starknet-api): derive serialize for executable transaction feat(starknet_api): derive serialize for executable transaction Aug 15, 2024
Copy link
Contributor

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

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

@MohammadNassar1 MohammadNassar1 merged commit c35d005 into main Aug 15, 2024
17 of 19 checks passed
@MohammadNassar1 MohammadNassar1 deleted the mohammad/executable-transaction/derive-serialize-and-deserialize branch August 15, 2024 08:09
@MohammadNassar1 MohammadNassar1 restored the mohammad/executable-transaction/derive-serialize-and-deserialize branch August 15, 2024 08:25
@ArniStarkware
Copy link
Contributor

crates/starknet_api/src/executable_transaction.rs line 64 at r2 (raw file):

    pub tx_hash: TransactionHash,
    pub class_info: ClassInfo,
}

If we want to use Blockifier's ClassInfo, we have a problem: it cannot derive serialize.

Code quote:

// TODO(Mohammad): Add constructor for all the transaction's structs.
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub struct DeclareTransaction {
    pub tx: crate::transaction::DeclareTransaction,
    pub tx_hash: TransactionHash,
    pub class_info: ClassInfo,
}

@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 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.

3 participants