Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions prdoc/pr_6027.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
title: Remove pallet::getter from pallet-offences
doc:
- audience: Runtime Dev
description: |
This PR removes pallet::getter from pallet-offences from type Reports. It also adds a test to verify that retrieval of Reports still works with storage::getter.

crates:
- name: pallet-offences
bump: patch
Comment thread
seadanda marked this conversation as resolved.
16 changes: 15 additions & 1 deletion substrate/frame/offences/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,28 @@ pub mod pallet {

/// The primary structure that holds all offence records keyed by report identifiers.
#[pallet::storage]
#[pallet::getter(fn reports)]
pub type Reports<T: Config> = StorageMap<
_,
Twox64Concat,
ReportIdOf<T>,
OffenceDetails<T::AccountId, T::IdentificationTuple>,
>;

impl<T: Config> Pallet<T> {
Comment thread
seadanda marked this conversation as resolved.
Outdated
#[deprecated(note = "Use Reports::<T>::get(id) instead.")]

@shawntabrizi shawntabrizi Oct 14, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but there is no need to deprecate here.

Just keep this public function around forever. The point is just to remove the macro code/logic, not necessarily remove this functionality.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah that was my request - that was the impression that I got from the discussion on the issue, maybe I'm not up to date on the intention behind removing the getters

pub fn reports<KArg>(
k: KArg,
) -> Option<OffenceDetails<T::AccountId, T::IdentificationTuple>>
where
KArg: frame_support::__private::codec::EncodeLike<ReportIdOf<T>>,
Comment thread
seadanda marked this conversation as resolved.
Outdated
{
<Reports<T> as frame_support::storage::StorageMap<
ReportIdOf<T>,
OffenceDetails<T::AccountId, T::IdentificationTuple>,
>>::get(k)
Comment thread
seadanda marked this conversation as resolved.
Outdated
}
}

/// A vector of reports of the same kind that happened at the same time slot.
#[pallet::storage]
pub type ConcurrentReportsIndex<T: Config> = StorageDoubleMap<
Expand Down
27 changes: 25 additions & 2 deletions substrate/frame/offences/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,35 @@

use super::*;
use crate::mock::{
new_test_ext, offence_reports, with_on_offence_fractions, Offence, Offences, RuntimeEvent,
System, KIND,
new_test_ext, offence_reports, with_on_offence_fractions, Offence, Offences, Runtime,
RuntimeEvent, System, KIND,
};
use frame_system::{EventRecord, Phase};
use sp_core::H256;
use sp_runtime::Perbill;

#[test]
fn should_get_reports_with_storagemap_getter_and_deprecated_getter() {
Comment thread
shawntabrizi marked this conversation as resolved.
Outdated
new_test_ext().execute_with(|| {
// given
let report_id: ReportIdOf<Runtime> = H256::from_low_u64_be(1);
let offence_details = OffenceDetails { offender: 1, reporters: vec![2, 3] };

Reports::<Runtime>::insert(report_id, offence_details.clone());

// when
#[allow(deprecated)]
Comment thread
shawntabrizi marked this conversation as resolved.
Outdated
let stored_offence_details = Offences::reports(report_id);
// then
assert_eq!(stored_offence_details, Some(offence_details.clone()));

// when
let stored_offence_details = Reports::<Runtime>::get(report_id);
// then
Comment thread
seadanda marked this conversation as resolved.
assert_eq!(stored_offence_details, Some(offence_details.clone()));
});
}

#[test]
fn should_report_an_authority_and_trigger_on_offence() {
new_test_ext().execute_with(|| {
Expand Down