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

Extract precompile-error crate #2300

Merged
merged 9 commits into from
Oct 16, 2024

Conversation

kevinheavey
Copy link

@kevinheavey kevinheavey commented Jul 26, 2024

Problem

Pulling out a solana_sdk::transaction crate requires pulling out solana_sdk::precompiles which requires pulling out solana_sdk::ed25519_instruction. But ed25519_instruction depends on the PrecompileError type from solana_sdk::precompiles, so PrecompileError needs to be pulled out to avoid a circular dependency

Summary of Changes

  • Move PrecompileError to its own crate
  • Remove thiserror from the new crate
  • Update PrecompileError usage in this repo
  • Re-export with deprecation

@kevinheavey kevinheavey force-pushed the precompile-error-crate branch 2 times, most recently from 5b42e9b to 26a8c80 Compare September 8, 2024 14:20
@kevinheavey kevinheavey force-pushed the precompile-error-crate branch from d5d147a to a559cfb Compare September 14, 2024 11:17
@kevinheavey kevinheavey force-pushed the precompile-error-crate branch 4 times, most recently from df21708 to 82a0067 Compare September 26, 2024 09:38
@kevinheavey kevinheavey force-pushed the precompile-error-crate branch 2 times, most recently from b9cb41a to 592b987 Compare October 10, 2024 22:31
@kevinheavey kevinheavey requested a review from febo October 10, 2024 23:11
@febo
Copy link

febo commented Oct 15, 2024

Looks good!

One minor nit – I think we need to update the import here to use the new crate: https://github.com/anza-xyz/agave/blob/master/programs/ed25519-tests/tests/process_transaction.rs#L8

I will wait for the crate so we can get all checks passing before approving – cc: @yihau.

@kevinheavey
Copy link
Author

kevinheavey commented Oct 15, 2024

One minor nit – I think we need to update the import here to use the new crate: https://github.com/anza-xyz/agave/blob/master/programs/ed25519-tests/tests/process_transaction.rs#L8

Yeah there are several places where the import should be updated but I prefer to do that separately otherwise this PR will get very big

@kevinheavey kevinheavey force-pushed the precompile-error-crate branch 2 times, most recently from 1ee08fe to e573787 Compare October 15, 2024 14:35
@febo
Copy link

febo commented Oct 15, 2024

@kevinheavey I think we only need to rebase and it should be good to go.

@kevinheavey kevinheavey force-pushed the precompile-error-crate branch from e573787 to 02fcb27 Compare October 15, 2024 15:46
@kevinheavey kevinheavey force-pushed the precompile-error-crate branch from 02fcb27 to 73a73c2 Compare October 15, 2024 19:22
@kevinheavey kevinheavey added the automerge automerge Merge this Pull Request automatically once CI passes label Oct 16, 2024
@febo febo merged commit a8aef04 into anza-xyz:master Oct 16, 2024
53 checks passed
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
* extract precompile-error crate

* update PrecompileError usage

* remove thiserror from precompile-error crate

* fmt

* remove num-derive

* fix imports after rebase

* sort deps

* sort deps

* fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge automerge Merge this Pull Request automatically once CI passes need:merge-assist
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants