Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThe PR switches the Rust toolchain to nightly and restructures trait-based extension design by removing associated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #561 +/- ##
=======================================
Coverage 96.08% 96.08%
=======================================
Files 54 54
Lines 5215 5215
=======================================
Hits 5011 5011
Misses 204 204 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Architecture.md`:
- Line 36: Update the sentence in Architecture.md to hyphenate the compound
adjective: change the phrase "polymorphism like behavior" to "polymorphism-like
behavior" so the line reads "...we achieve polymorphism-like behavior through
**associated types** and **trait bounds**."
🧹 Nitpick comments (2)
.github/workflows/generic.yml (1)
49-51: Mirror the pinned nightly here once you select it.When you pin nightly in
rust-toolchain.toml, update thistoolchain:value to the same date to prevent local/CI mismatches.rust-toolchain.toml (1)
2-2: Pin nightly to a tested date for reproducibility and stable CI builds.The codebase uses
#![feature(negative_impls)], which requires the nightly channel. Since unpinned nightly can introduce unexpected compiler changes, specify a concrete nightly version (e.g.,channel = "nightly-2026-02-01") to ensure consistent builds across environments.
|
great work, this is very elegant but have a couple of concerns. Using this feature requires the nightly toolchain, but I don't think this is a common practice in the ecosystem. I tested with |
Why should it? Nightly is compatible with stable. The deal breaker would be reverse scenario, where stellar_sdk uses nightly features, and we force our users to use the stable version. Notice that soroban_sdk does not require us to use nightly, it is based on stable, yet we can compile everything completely fine by using nightly. I don't see a problem tbh |
|
On top of that, cargo automatically handles the change behind the scenes. Having some projects using nightly, and some using stable, at the same time, is not that uncommon in rust. The end developers may not even notice cargo is using nightly in the background |
|
the error I get: |
|
that's weird. Because I didn't put any command on my workflow to change the toolchain. I just updated the toolchain file in the repo, and voila! Let's go to slack for discussing this further |
Fixes #560
PR Checklist
P.S. strongly suggest to run
cargo cleanon your workspace after this merges, because we are switching toolchain. This will prevent non-sense weird error messages. Speaking from experience :')Summary by CodeRabbit
Documentation
New Features
allowed(),allow_user(), anddisallow_user()for managing permitted accounts.Refactor