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: to_rpc_contract_class is still returning contracts with a lot of missing data #1316

Open
tdelabro opened this issue Dec 13, 2023 · 27 comments
Labels
backlog Ready to be picked enhancement New feature or request needed-for-mainnet

Comments

@tdelabro
Copy link
Collaborator

/// Returns a [`ContractClass`] from a [`BlockifierContractClass`]
pub fn to_rpc_contract_class(contract_class: BlockifierContractClass) -> Result<ContractClass> {
    match contract_class {
        BlockifierContractClass::V0(contract_class) => {
            let entry_points_by_type = to_legacy_entry_points_by_type(&contract_class.entry_points_by_type)?;
            let compressed_program = compress(&contract_class.program.to_bytes())?;
            Ok(ContractClass::Legacy(CompressedLegacyContractClass {
                program: compressed_program,
                entry_points_by_type,
                // FIXME 723
                abi: None,
            }))
        }
        BlockifierContractClass::V1(_contract_class) => Ok(ContractClass::Sierra(FlattenedSierraClass {
            sierra_program: vec![], // FIXME: https://github.com/keep-starknet-strange/madara/issues/775
            contract_class_version: option_env!("COMPILER_VERSION").unwrap_or("0.11.2").into(),
            entry_points_by_type: EntryPointsByType { constructor: vec![], external: vec![], l1_handler: vec![] }, /* TODO: add entry_points_by_type */
            abi: String::from("{}"), // FIXME: https://github.com/keep-starknet-strange/madara/issues/790
        })),
    }
}

This piece of code is still missing most fields. How to solve this?

Possible solution:

  • store the data lost when passed to the runtime (eg. abi) in a kvStorage in the client, read them back from there when answering get_class rpc request
@tdelabro tdelabro added the enhancement New feature or request label Dec 13, 2023
@Juul-Mc-Goa
Copy link
Contributor

Hi, would be interested to work on this.

@tdelabro
Copy link
Collaborator Author

Thanks a lot @Juul-Mc-Goa ! Message me if you don't know how to design things.

@tdelabro
Copy link
Collaborator Author

Hey @Juul-Mc-Goa, this one had been assigned to you but I don't think you ever worked on it.
Do you want to, or can I unassign you?

@tdelabro tdelabro added the backlog Ready to be picked label Jan 19, 2024
@Juul-Mc-Goa
Copy link
Contributor

Hey, sorry got busy with other things.
I can start working on it next monday, or you can unassign me if you prefer.

@tdelabro
Copy link
Collaborator Author

Starting next Monday will be great! Thank you

@tdelabro
Copy link
Collaborator Author

make sure to read #790 as it is part of what you will be doing

@Juul-Mc-Goa
Copy link
Contributor

Hey @tdelabro, just to be sure, the kvStorage in the client will not be gossiped to other nodes ?
If it has to be gossiped, as said in #775, then I don't know how it should be implemented.

@tdelabro
Copy link
Collaborator Author

tdelabro commented Jan 22, 2024

@Juul-Mc-Goa indeed it is not gossiped. We have to find some other way.
Starknet does not have the issue coz it is centralized.
I think we have to gossip them or make them available one way or another.

Put your work here in hold, and open a discussion so that we can find the best way to do that.
List the potential solutions you can think as a starting point for the discussion.

@Juul-Mc-Goa
Copy link
Contributor

@tdelabro started a discussion there

@tdelabro tdelabro assigned tdelabro and unassigned Juul-Mc-Goa Jan 27, 2024
@tdelabro
Copy link
Collaborator Author

I'm working on this whole flow. There is a number of task to do first, but I'm on it

Copy link

There hasn't been any activity on this issue recently, and in order to prioritize active issues, it will be marked as stale.
Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a 👍
Because this issue is marked as stale, it will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions bot added the stale label Feb 27, 2024
@tdelabro
Copy link
Collaborator Author

We now have contract classes stored in the client (since #1409).
So I think it reopen the possibility to improve this part

wanna give it a try @Juul-Mc-Goa ?

@github-actions github-actions bot removed the stale label Feb 29, 2024
@tdelabro tdelabro removed their assignment Mar 8, 2024
Copy link

github-actions bot commented Apr 8, 2024

There hasn't been any activity on this issue recently, and in order to prioritize active issues, it will be marked as stale.
Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a 👍
Because this issue is marked as stale, it will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions bot added the stale label Apr 8, 2024
@elielnfinic
Copy link
Contributor

has this been fixed? @tdelabro @Juul-Mc-Goa

@github-actions github-actions bot removed the stale label Apr 16, 2024
@tdelabro
Copy link
Collaborator Author

@elielnfinic nope, still an issue

@elielnfinic
Copy link
Contributor

I will come back to this after completing the other task you just assigned to me. If you don't mind, you can assign this to me too.

@elielnfinic
Copy link
Contributor

Has this function been changed to blockifier_to_rpc_contract_class_types in crates/client/rpc-core/src/utils.rs?

@tdelabro
Copy link
Collaborator Author

tdelabro commented May 6, 2024

@elielnfinic yes indeed

@elielnfinic
Copy link
Contributor

Is conversion from EntryPointV1 to SierraEntryPoint available?

@elielnfinic
Copy link
Contributor

I think, I am able to fix that conversion.

@tdelabro
Copy link
Collaborator Author

@elielnfinic I think this PR will be fixed by some work I'm doing, storing the casm contract class on the client.
It will provide all the data required to fill those missing fields. I don't think you will be able to complete this issue without this

@elielnfinic
Copy link
Contributor

I can create a draft PR to see if some parts of my code be used.

@tdelabro
Copy link
Collaborator Author

Sure, but don't spend too much time on that :)

@elielnfinic
Copy link
Contributor

Hey @tdelabro, please proceed, I will not submit the PR draft.

Copy link

There hasn't been any activity on this issue recently, and in order to prioritize active issues, it will be marked as stale.
Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a 👍
Because this issue is marked as stale, it will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions bot added the stale label Jun 15, 2024
@Juul-Mc-Goa
Copy link
Contributor

Hey,

Is this issue still relevant ? I would be interested to work on this.

@github-actions github-actions bot removed the stale label Jun 23, 2024
@tdelabro
Copy link
Collaborator Author

I'm on it #1631

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Ready to be picked enhancement New feature or request needed-for-mainnet
Projects
Status: 🏗 In progress
Development

No branches or pull requests

4 participants