chore: Move some sdk::core types to their own crate, sdk-core-types
#1693
Conversation
codeblooded1729
changed the title
sdk::core types to its own cratesdk::core types to their own crate, sdk-core-types
codeblooded1729
force-pushed
the
kapil/sdk_core_types
branch
from
b7c0491 to
01cd2a2
Compare
codeblooded1729
requested review from
spiral-ladder,
supragya and
matthiasgoergens
| edition = "2021" | ||
| keywords = ["sdk", "types"] | ||
| license = "Apache-2.0" | ||
| name = "sdk-core-types" |
There was a problem hiding this comment.
| name = "sdk-core-types" | |
| name = "core" |
I'd argue you could even just name this core. ecall IDs and poseidon2 constants aren't really types, and they aren't really constrained to SDK usage only.
There was a problem hiding this comment.
core sounds like functionality. These are not such elements if I understand correctly.
There was a problem hiding this comment.
You are right. Creating a separate crate for extracting out constants doesn't seem like the best thing to do.
| @@ -2,45 +2,16 @@ | |||
| use core::arch::asm; | |||
|
|
|||
| #[cfg(target_os = "mozakvm")] | |||
There was a problem hiding this comment.
One of the benefits of moving the consts and the log fn out of this file is that we can now put this cfg file-wide at the top of the file:
#![cfg(target_os = "mozakvm")]And remove all the per fn/import configuration.
|
|
||
| #[must_use] | ||
| pub fn log<'a>(raw_id: u32) -> &'a str { |
There was a problem hiding this comment.
IMO, please either:
- provide a doc comment on what this is meant to log,
- or just put this fn within
ecall(), since that's the only place this is used. Then there would be no ambiguity in what this fn is meant to log.
| use mozak_runner::reg_abi::REG_A1; | ||
| use sdk_core_types::constants::poseidon2::DIGEST_BYTES; |
| @@ -523,7 +523,7 @@ pub fn create_poseidon2_test( | |||
| op: Op::ADD, | |||
| args: Args { | |||
| rd: REG_A0, | |||
| imm: ecall::POSEIDON2, | |||
| imm: ecall_id::POSEIDON2, | |||
There was a problem hiding this comment.
We moved from ecall_id -> ecall -> ecall_id again
| plonky2 = { workspace = true, default-features = false } | ||
| proptest = { version = "1.4", optional = true } | ||
| sdk-core-types = { path = "../sdk-core-types" } |
| @@ -0,0 +1,4 @@ | |||
| #![cfg_attr(target_os = "mozakvm", no_std)] | |||
|
|
|||
| pub mod constants; | |||
There was a problem hiding this comment.
pub mod poseidon2hash maybe.
|
The goal of this PR was to break the dependency of |
Main benefit is that
circuitsandrunnerno longer havesdkas dependency.