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

[pr] Borsh Serialize solana_sdk::Transaction #8

Merged
merged 20 commits into from
Sep 24, 2024

Conversation

DudessaPr
Copy link

@DudessaPr DudessaPr commented Sep 18, 2024

Problem

In svm-rollup pr we are using Transaction inside of CallMessage, which requires Borsh

Summary of Changes

Added BroshSerialization/Deserialization to Transaction struct, and other structs inside of it, such as Signature, CompiledInstruction, etc.

Fixes #

@DudessaPr DudessaPr changed the title Borsh serialize transaction [pr] Borsh Serialize solana_sdk::Transaction Sep 19, 2024
@DudessaPr DudessaPr changed the title [pr] Borsh Serialize solana_sdk::Transaction [wip] Borsh Serialize solana_sdk::Transaction Sep 19, 2024
}

// Implement Arbitrary for Signature
impl<'a> arbitrary::Arbitrary<'a> for Signature {

Choose a reason for hiding this comment

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

Just a Rust language question.. was the lifetime annotation required here? I would've thought this was a case where it was just implicit.

Copy link
Author

@DudessaPr DudessaPr Sep 19, 2024

Choose a reason for hiding this comment

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

It was required by trait definition of arbitrary::Arbitrary
pub trait Arbitrary<'a>: Sized {

@DudessaPr DudessaPr changed the title [wip] Borsh Serialize solana_sdk::Transaction [pr] Borsh Serialize solana_sdk::Transaction Sep 19, 2024
@Yiwen-Gao
Copy link

Can you elaborate on why Arbitrary and Proptest are needed? It'd be helpful to link code snippets as well.

@Yiwen-Gao
Copy link

They're testing libraries, so even if we do need them, I believe they should be gated behind configuration flags.

@DudessaPr
Copy link
Author

Can you elaborate on why Arbitrary and Proptest are needed? It'd be helpful to link code snippets as well.

I see some sovereign modules use Arbitrary and Proptest for fuzz testing. And in the module template we had those created derived, I mean they had the following in callMessage

#[cfg_attr(
    feature = "arbitrary",
    derive(arbitrary::Arbitrary, proptest_derive::Arbitrary)
)]

I believe we will need those crates in the future for more testing to find bugs in the module. Therefore, decided not to remove it from CallMessage and implement it for the Transaction type.

@DudessaPr
Copy link
Author

They're testing libraries, so even if we do need them, I believe they should be gated behind configuration flags.

You are right, I missed adding flags here. I did not think about them and totally forgot. Will change that

@DudessaPr DudessaPr merged commit d3b8cb0 into feat/risc0 Sep 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants