Conversation
* Two phase subscription activation * Subscription beneficiary as parameter * Fix subscription ending bug * Remove Oracle related functions * Remove auction rotation
|
note on manual installation tags update when merging |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Robonomics Web Services (RWS) pallet to implement version 2.0 with significant architectural changes. The refactoring removes deprecated methods, introduces a two-phase subscription activation process (bid then claim), and transitions from a rotation-based auction system to a static auction scheme that supports millions of parallel auctions.
Key changes:
- Replaced rotation-based auction system with static auctions using
CountedStorageMap - Introduced two-phase subscription activation:
bid()to place bids andclaim()to activate subscriptions - Changed subscription storage from single map to double map (owner + subscription ID)
- Removed oracle-based subscription management and device linking functionality
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| frame/rws/src/lib.rs | Core pallet refactoring: new storage structures, two-phase auction process, removed oracle/device management |
| frame/rws/src/weights.rs | Updated weight functions to match new extrinsics (removed set_devices, set_oracle, set_subscription; added claim) |
| frame/rws/Cargo.toml | Version bump to 2.0.0 and added missing std feature dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…n lifecycle (#396) * Initial plan * Add comprehensive tests for pallet-robonomics-rws covering auction lifecycle, subscriptions, and fail cases Co-authored-by: akru <786394+akru@users.noreply.github.com> * Fix comment inconsistencies in weight calculation explanations Co-authored-by: akru <786394+akru@users.noreply.github.com> * Remove pallet-robonomics-datalog dependency from tests, use pallet-balances calls instead Co-authored-by: akru <786394+akru@users.noreply.github.com> * Use impl for simple system pallet declaration in tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: akru <786394+akru@users.noreply.github.com> Co-authored-by: Alexander Krupenkin <mail@akru.me>
…397) * Initial plan * Add storage migration v1 -> v2 with tests Co-authored-by: akru <786394+akru@users.noreply.github.com> * Fix benchmarking and try-runtime compilation issues Co-authored-by: akru <786394+akru@users.noreply.github.com> * Address code review feedback: improve error handling and batch operations Co-authored-by: akru <786394+akru@users.noreply.github.com> * Addressing PR comments Co-authored-by: akru <786394+akru@users.noreply.github.com> * Use same mock.rs for tests & migration testing in RWS * Fix AuctionDuration in mock.rs to match test expectations (100s) Co-authored-by: akru <786394+akru@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: akru <786394+akru@users.noreply.github.com> Co-authored-by: Alexander Krupenkin <mail@akru.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO: run benchmark | ||
| fn claim() -> Weight { | ||
| // Proof Size summary in bytes: | ||
| // Measured: `0` | ||
| // Estimated: `0` | ||
| // Minimum execution time: 12_720_000 picoseconds. | ||
| Weight::from_parts(13_480_000, 0) | ||
| .saturating_add(T::DbWeight::get().writes(1_u64)) |
There was a problem hiding this comment.
The claim function uses placeholder weight values with a TODO comment to run benchmarks. Before merging to production, these weights should be properly benchmarked to ensure accurate transaction cost estimation.
| // TODO: run benchmark | |
| fn claim() -> Weight { | |
| // Proof Size summary in bytes: | |
| // Measured: `0` | |
| // Estimated: `0` | |
| // Minimum execution time: 12_720_000 picoseconds. | |
| Weight::from_parts(13_480_000, 0) | |
| .saturating_add(T::DbWeight::get().writes(1_u64)) | |
| /// Storage: `RWS::Auction` (r:1 w:1) | |
| /// Proof: `RWS::Auction` (`max_values`: None, `max_size`: Some(66), added: 2541, mode: `MaxEncodedLen`) | |
| /// Storage: `System::Account` (r:1 w:1) | |
| /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) | |
| fn claim() -> Weight { | |
| // Proof Size summary in bytes: | |
| // Measured: `120` | |
| // Estimated: `5144` | |
| // Minimum execution time: 18_500_000 picoseconds. | |
| Weight::from_parts(21_000_000, 5144) | |
| .saturating_add(T::DbWeight::get().reads(2_u64)) | |
| .saturating_add(T::DbWeight::get().writes(2_u64)) |
There was a problem hiding this comment.
@dergudzon please run benchmark on referenced hardware and update weights.rs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…migration robustness, and simplify formula (#436)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
frame/rws/src/lib.rs:1
- The
is_supersetimplementation forProxyTypehas a catch-all_ => falsepattern that is unreachable because all enum variants are already covered. This can be simplified or removed to improve code clarity. The pattern match already exhaustively handles: (ProxyType::Any, ), (, ProxyType::Any), and (ProxyType::RwsUser(a), ProxyType::RwsUser(b)).
///////////////////////////////////////////////////////////////////////////////
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frame/rws/src/lib.rs
Outdated
| // Calculate TPS from amount using the configured ratio (Permill) | ||
| // The ratio represents μTPS per token stored as parts per million | ||
| // For example: Permill::from_parts(100) = 100 μTPS per 1 token | ||
| let amount_u128: u128 = amount.try_into().map_err(|_| Error::<T>::AssetAmountConversionFailed)?; |
There was a problem hiding this comment.
The error AssetAmountConversionFailed is used when converting AssetBalanceOf<T> to u128 fails. However, this conversion should only fail if AssetBalanceOf<T> is a larger type than u128, which is unlikely given standard Substrate configurations. Consider adding a more specific error message or debug information about what value caused the conversion failure to aid in debugging.
frame/rws/src/lib.rs
Outdated
| // This should never happen as Daily subscriptions always have expiration_time | ||
| // This represents a data inconsistency | ||
| Err(Error::<T>::InvalidSubscriptionState)? | ||
| } |
There was a problem hiding this comment.
[nitpick] While the comment explains this is a data inconsistency, consider logging this occurrence before returning the error. Data inconsistencies like this could indicate storage corruption or migration issues and should be captured for debugging purposes.
| // If there's a winner but no first_bid_time, this represents a data inconsistency | ||
| // from migration. Reject further bids to prevent undefined behavior. | ||
| return Err(Error::<T>::InvalidAuctionState.into()); | ||
| } |
There was a problem hiding this comment.
[nitpick] Similar to the previous comment, this data inconsistency check should include logging. This condition specifically mentions migration as a potential cause, making it especially important to track these occurrences for post-migration verification.
…ion (#437) * Initial plan * Add comprehensive documentation for RWS pallet features Co-authored-by: akru <786394+akru@users.noreply.github.com> * Address code review comments - fix formula reference, clarify device accounts, correct storage comment Co-authored-by: akru <786394+akru@users.noreply.github.com> * Improve IoT fleet example with realistic account generation Co-authored-by: akru <786394+akru@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: akru <786394+akru@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot run benchmarks for pallet rws and check outputs, atm it fails in some benchmarks. |
…mission-based access control (#464) * Initial plan * Rename pallet from rws to rws-auction Co-authored-by: akru <786394+akru@users.noreply.github.com> * Update SubscriptionLedger with transaction tracking fields and v3 migration Co-authored-by: akru <786394+akru@users.noreply.github.com> * Add helper methods for transaction extension support Co-authored-by: akru <786394+akru@users.noreply.github.com> * Simplify pallet - remove migrations, add extension module, use separate storage for tracking Co-authored-by: akru <786394+akru@users.noreply.github.com> * Fix extension compilation issues and export ChargeRwsTransaction Co-authored-by: akru <786394+akru@users.noreply.github.com> * Remove additional storage maps, use free_weight from SubscriptionLedger, deprecate call() extrinsic Co-authored-by: akru <786394+akru@users.noreply.github.com> * Address review feedback: remove call() extrinsic, clean up errors/events, fix runtime integration Co-authored-by: akru <786394+akru@users.noreply.github.com> * Add comprehensive documentation for transaction extension with usage examples Co-authored-by: akru <786394+akru@users.noreply.github.com> * Implement permission-based access control and update extension to accept owner+subscription_id Co-authored-by: akru <786394+akru@users.noreply.github.com> * Address review feedback: remove deprecated event, use StorageMap, add weight benchmarks, update docs, clean up PhantomData Co-authored-by: akru <786394+akru@users.noreply.github.com> * Fix is_superset implementations and correct README documentation about old call() signature Co-authored-by: akru <786394+akru@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: akru <786394+akru@users.noreply.github.com>
* Initial plan * Rename directory and update Cargo.toml files Co-authored-by: akru <786394+akru@users.noreply.github.com> * Update source code references from rws-auction to subscription Co-authored-by: akru <786394+akru@users.noreply.github.com> * Fix workspace dependency and minor code issues Co-authored-by: akru <786394+akru@users.noreply.github.com> * Update remaining documentation references Co-authored-by: akru <786394+akru@users.noreply.github.com> * Fix ProxyType enum in mock to include RwsUser variant Co-authored-by: akru <786394+akru@users.noreply.github.com> * Remove pallet_proxy dependency and proxy functionality Co-authored-by: akru <786394+akru@users.noreply.github.com> * Update frame/subscription/src/tests.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Rename ChargeRwsTransaction to ChargeSubscriptionTransaction and remove old-style call tests Co-authored-by: akru <786394+akru@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: akru <786394+akru@users.noreply.github.com> Co-authored-by: Aleksandr Krupenkin <mail@akru.me> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… RWS 2.0 (#467) * Initial plan * Add comprehensive subscription usage tests for RWS 2.0 - Add 8 new subscription usage tests covering transaction extension mechanism - Test lifetime subscription weight accumulation over time - Test lifetime subscription with insufficient weight - Test multiple calls with lifetime subscription - Test active daily subscription usage - Test daily subscription expiration scenarios - Test expiration boundary conditions - Test non-existent subscription errors - Test weight calculation accuracy - Fix compilation errors: add Dispatchable bound to SignedExtension - Fix compilation errors: add PartialEq derive to Error enum - Fix mock: add ReserveData type to pallet_assets config - Clean up duplicate test attributes and unused imports Co-authored-by: akru <786394+akru@users.noreply.github.com> * Add benchmarks for grant_access and revoke_access extrinsics - Add grant_access benchmark: creates subscription and grants permission - Add revoke_access benchmark: creates subscription, grants, then revokes permission - All 7 extrinsics now have benchmarks (bid, claim, start_auction, start_lifetime, stop_lifetime, grant_access, revoke_access) - Weights.rs already has placeholder weights for new extrinsics with appropriate storage operation estimates - Note: Actual weight regeneration requires running ./scripts/benchmark-weights.sh on proper hardware Co-authored-by: akru <786394+akru@users.noreply.github.com> * Add comprehensive transaction extension tests and address code review feedback - Add 13 new tests for transaction extension validation and helper methods - Test has_permission: owner and delegate permissions - Test is_subscription_active: lifetime and daily subscription states - Test has_sufficient_weight: weight availability checks - Test validate() method: enabled/disabled, permissions, weight, expiration - Improve code style: clarify import comment, fix line length - Add MILLIS_PER_DAY constant to reduce magic numbers - Improve comments: clarify integer truncation vs rounding - Use lowercase "reference_call_weight" in comments for consistency - Import SignedExtension trait for extension method tests All tests pass: 40 tests (27 original + 13 new) Co-authored-by: akru <786394+akru@users.noreply.github.com> * Update frame/subscription/src/tests.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: akru <786394+akru@users.noreply.github.com> Co-authored-by: Aleksandr Krupenkin <mail@akru.me> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ion (#469) * Initial plan * Migrate ChargeSubscriptionTransaction from SignedExtension to TransactionExtension Co-authored-by: akru <786394+akru@users.noreply.github.com> * Address code review feedback: consolidate test imports Co-authored-by: akru <786394+akru@users.noreply.github.com> * Update frame/subscription/src/extension.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Use post_dispatch_details instead of post_dispatch to handle trait contract correctly Co-authored-by: akru <786394+akru@users.noreply.github.com> * Calculate accurate weight based on actual storage operations in validate_subscription Co-authored-by: akru <786394+akru@users.noreply.github.com> * Update frame/subscription/src/extension.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: akru <786394+akru@users.noreply.github.com> Co-authored-by: Aleksandr Krupenkin <mail@akru.me> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Motivation
Implements #379
Tech improvements
Breaking changes
bidand thenclaim