Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

SDK: refactor Signer and Signers traits #34984

Merged
merged 6 commits into from
Mar 1, 2024

Conversation

billythedummy
Copy link
Contributor

@billythedummy billythedummy commented Jan 27, 2024

Problem

See #34983

Summary of Changes

  • Remove From blanket impl for Signer
  • add ?Sized trait bound to blanket impl for Signer for Box<dyn Signer>
  • retain Deref blanket impl for Signer
  • refactor Signers to blanket impl for all types where their references impls IntoIterator yielding Signer refs

Fixes #34983
Fixes #13670

@mergify mergify bot added community Community contribution need:merge-assist labels Jan 27, 2024
@mergify mergify bot requested a review from a team January 27, 2024 06:06
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 12, 2024
@github-actions github-actions bot closed this Feb 20, 2024
@joncinque joncinque reopened this Feb 21, 2024
@joncinque joncinque added the CI Pull Request is ready to enter CI label Feb 21, 2024
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Feb 21, 2024
@joncinque joncinque added CI Pull Request is ready to enter CI and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Feb 21, 2024
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Feb 21, 2024
@joncinque
Copy link
Contributor

Sorry for the lateness on the review: this comes in at a great time, since we're planning on moving to 2.0 and removing old stuff anyway, so we should be able to accept the breaking change of removing the blanket From impl.

The change looks good, and I'll be so happy to avoid having to write as_ref() everywhere, it's been such an annoyance for a long time.

Can you rebase your branch to see what breaks downstream?

@billythedummy
Copy link
Contributor Author

Can you rebase your branch to see what breaks downstream?

Just merged master into this branch. Building takes too long on my machine so I'm just using the CI checks to see what breaks haha

@CriesofCarrots
Copy link
Contributor

Just merged master into this branch. Building takes too long on my machine so I'm just using the CI checks to see what breaks haha

We much prefer rebases to merge commits.

@joncinque joncinque added the CI Pull Request is ready to enter CI label Feb 28, 2024
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Feb 28, 2024
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

The changes look good to me! I'm very pleasantly surprised that this just works ™️ for the downstream builds. I think we use Arc<&dyn Signer> everywhere in those projects.

Higher-rank trait bounds always confuse me, so I tried to make this work without a higher-rank trait bound, but it seems like we need it in this case since all of the functions are implemented on &self, which means we need some higher bound on the lifetime of the self reference.

What's really nice about this implementation is that it's bound to IntoIterator, which is typically much easier to use than Iterator, and yet it doesn't consume the underlying object, so I can call any of the functions, like sign_message, multiple times, with the same Vec[&dyn Signer].

Anyway, great work!

@joncinque joncinque merged commit e3b9d7f into solana-labs:master Mar 1, 2024
47 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Community contribution need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDK: Signer and Signers traits are problematic Improve Signer trait, documentation
4 participants