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

Pull sanitize crate out of solana-program #1707

Merged
merged 8 commits into from
Jun 29, 2024

Conversation

kevinheavey
Copy link

Problem

The Sanitize trait is a de facto internal trait that doesn't belong in solana-program, and it makes it harder to move a bunch of other things out of solana-program

Summary of Changes

Move sanitize.rs into a new solana-sanitize crate.

@kevinheavey kevinheavey force-pushed the sanitize-crate branch 2 times, most recently from 66570e1 to 44c2408 Compare June 18, 2024 14:32
Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

So, just to summarize, we're introducing a leaf-node crate for solana-santize.

The monorepo packages which need the Sanitize trait are:

  • bloom
  • core
  • gossip
  • version

The Sanitize trait itself is designed for sanitizing over-the-wire message payloads, and >90% of downstream user cases of SDK and program SDK, will never need sanitize().

Regardless, here they are listed for posterity:

  • blake3 hashing
  • hash (sha2) hashing
  • keccak hashing
  • CompiledInstruction
  • Pubkey
  • Signature
  • Almost all transaction message types
  • Almost all transaction types

I'm in favor of this change. I think it makes sense for networking-related libraries to import solana-sanitize if they need to rely on sanitized messages, and not to impose this requirement on other users of SDK types.

sanitize/Cargo.toml Outdated Show resolved Hide resolved
buffalojoec
buffalojoec previously approved these changes Jun 21, 2024
Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Lgtm @kevinheavey, thanks for the continued crusade on dependencies!

On crates.io, add anza-team and pull yourself as owner, and then retry the CI job.

@kevinheavey
Copy link
Author

Added, will remove myself as owner once anza-team accepts the ownership invite https://crates.io/crates/solana-sanitize/

Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Looks good, just change the deprecated "since" version.

sdk/program/src/lib.rs Show resolved Hide resolved
Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks!

@buffalojoec buffalojoec added the automerge automerge Merge this Pull Request automatically once CI passes label Jun 28, 2024
@mergify mergify bot removed the automerge automerge Merge this Pull Request automatically once CI passes label Jun 28, 2024
Copy link

mergify bot commented Jun 28, 2024

automerge label removed due to a CI failure

@kevinheavey kevinheavey added the automerge automerge Merge this Pull Request automatically once CI passes label Jun 29, 2024
@mergify mergify bot merged commit a4ba708 into anza-xyz:master Jun 29, 2024
53 checks passed
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants