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 decode-error from solana-program #1813

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

kevinheavey
Copy link

Problem

The DecodeError trait in decode_error.rs gets used all over solana-program, making it hard to move anything out of solana-program.

Summary of Changes

  • Move decode_error.rs to its own crate
  • Re-export solana_decode_error as decode_error in solana-program for backwards compatibility

@kevinheavey kevinheavey force-pushed the extract-decode-error branch 4 times, most recently from d14b1c1 to 5fd6dc0 Compare July 1, 2024 11:46
@kevinheavey kevinheavey force-pushed the extract-decode-error branch 2 times, most recently from a1731c8 to b967e9c Compare July 4, 2024 09:07
@kevinheavey kevinheavey requested a review from buffalojoec July 4, 2024 11:23
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.

It appears to me that we can instead just eliminate this trait.

The decode_custom_error_to_enum is just indirection around FromPrimitive::from_u32, and it's only used in tests and impl PrintProgramError for ProgramError, which can just use FromPrimitive directly.

I also can't see where fn type_of() is used anymore. I can take the DecodeError impl out of some builtin program errors without issue, and I can change it within SPL programs and it doesn't affect anything as far as I can tell.

The direction would then be to use FromPrimitive::from_u32 directly within impl PrintProgramError for ProgramError and then deprecate the trait. What do you think?

@kevinheavey
Copy link
Author

@buffalojoec makes sense, though CI fails when I do that because it requires a change to spl-token too:

error[E0276]: impl has stricter requirements than trait
   --> ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/spl-token-6.0.0/src/error.rs:100:15
    |
100 |             + DecodeError<E>
    |               ^^^^^^^^^^^^^^ impl has extra requirement `E: DecodeError<E>`

The PR to deprecate DecodeError is here #2046

@buffalojoec
Copy link

@buffalojoec makes sense, though CI fails when I do that because it requires a change to spl-token too:

error[E0276]: impl has stricter requirements than trait
   --> ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/spl-token-6.0.0/src/error.rs:100:15
    |
100 |             + DecodeError<E>
    |               ^^^^^^^^^^^^^^ impl has extra requirement `E: DecodeError<E>`

The PR to deprecate DecodeError is here #2046

Yeah that's expected. I left a comment over on that PR about taking a little slower deprecation approach. We can maybe leave this one open for a bit in case we decide it's better to go with the extracted crate.

@kevinheavey
Copy link
Author

If we leave the DecodeError implementations in then we should still move it to a new crate imo, otherwise we can't do any more SDK splitting for a year or however long it is until the 3.0 release

@kevinheavey kevinheavey force-pushed the extract-decode-error branch 4 times, most recently from 44ba209 to 3212858 Compare July 16, 2024 19:07
buffalojoec
buffalojoec previously approved these changes Jul 17, 2024
@kevinheavey kevinheavey added the automerge automerge Merge this Pull Request automatically once CI passes label Jul 17, 2024
@mergify mergify bot merged commit 31012e3 into anza-xyz:master Jul 17, 2024
54 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
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants