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

Helios sp1 integration #689

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Helios sp1 integration #689

wants to merge 20 commits into from

Conversation

0xSasaPrsic
Copy link
Member

@0xSasaPrsic 0xSasaPrsic commented Nov 19, 2024

Pull Request type

Helios sp1 integration.
https://github.com/succinctlabs/sp1-helios/blob/main/contracts/src/SP1LightClient.sol

Please add the labels corresponding to the type of changes your PR introduces:

  • Feature
  • Bugfix
  • Refactor
  • Format
  • Documentation
  • Testing
  • Other:

Description

Related Issues

Testing Performed

Checklist

  • I have performed a self-review of my own code.
  • The tests pass successfully with cargo test.
  • The code was formatted with cargo fmt.
  • The code compiles with no new warnings with cargo build --release and cargo build --release --features runtime-benchmarks.
  • The code has no new warnings when using cargo clippy.
  • If this change affects documented features or needs new documentation, I have created a PR with a documentation update.

@@ -20,6 +20,9 @@ sp-runtime = { workspace = true, default-features = false }
sp-std = { workspace = true, default-features = false }
sp-core = { workspace = true, default-features = false }
frame-benchmarking = { workspace = true, default-features = false, optional = true }
# TODO is there a release version?

Choose a reason for hiding this comment

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

The 4.0.0-rc1 tag should be out in the next few days, which you can use. For now, I'd recommend pinning to a specific commit.

@@ -38,6 +41,7 @@ ethabi.workspace = true
[dev-dependencies]
pallet-balances = { workspace = true, default-features = false, features = ["std"] }
pallet-timestamp = { workspace = true, default-features = false }
sp1-sdk = { git = "https://github.com/succinctlabs/sp1", branch = "dev", default-features = false}

Choose a reason for hiding this comment

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

Same here.

@@ -164,13 +164,16 @@ pub const ROTATE_VK: &str = r#"{"vk_json":{
]
}}"#;

const SP1_VERIFICATION_KEY: [u8; 32] =

Choose a reason for hiding this comment

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

Add a description for how this is generated.

);
ensure!(is_valid.is_ok(), Error::<T>::VerificationFailed);

Head::<T>::set(new_head);

Choose a reason for hiding this comment

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

This seems correct.

@@ -232,9 +232,9 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
.unwrap();

vector_bridge::GenesisConfig::<Test> {
finality_threshold: 461,
// finality_threshold: 461,

Choose a reason for hiding this comment

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

nit: These should be deleted, rather than commented.

});
}

// #[test]

Choose a reason for hiding this comment

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

delete empty test

Copy link
Contributor

@Leouarz Leouarz left a comment

Choose a reason for hiding this comment

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

LGTM with overall looking since i don't have all the context.
2 libs still need real version instead of revisions.
Are benchmarks done in the reference machine ?

@0xSasaPrsic
Copy link
Member Author

LGTM with overall looking since i don't have all the context. 2 libs still need real version instead of revisions. Are benchmarks done in the reference machine ?

Correct, it needs running benchmarks on the referent machine and re-running build API once the review changes are done on the pallet.

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.

6 participants