-
Notifications
You must be signed in to change notification settings - Fork 6
Align Types between Bulletin and SDK #128
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
Conversation
|
|
||
| /// Storage period for data in blocks. Should match `sp_storage_proof::DEFAULT_STORAGE_PERIOD` | ||
| /// for block authoring. | ||
| #[pallet::storage] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkontur Shall we use #[pallet::storage] like on the SDK repo or keep as #[pallet::constant]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkontur Shall we use
#[pallet::storage]like on the SDK repo or keep as#[pallet::constant]?
hmm, good question, I don't know now, what we use at the end, it is kind of sensible constant, depends on pruning and renewal stuff, but want to have this as a configurable. We will later need to read this by custom inherent provider with runtime api.
So it does not matter if it is storage or const. Hmm, let's keep polkadot-sdk as storage and bulletin as constant for now, when we come to the provider, we will decide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. We can keep them as they are for now if there’s a difference between the two repos.
There is one tricky bit though: I think StoragePeriod may need to switch to #[pallet::storage]. In the SDK it’s initialised in genesis_build:
ByteFee::<T>::put(&self.byte_fee);
EntryFee::<T>::put(&self.entry_fee);
StoragePeriod::<T>::put(&self.storage_period);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For StoragePeriod, I’ll follow up on this in a separate PR.
pallets/common/src/lib.rs
Outdated
| for NoCurrency<AccountId, HoldReason> | ||
| { | ||
| type Balance = u128; | ||
| type Balance = u64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raymondkfcheung just out of curiosity, why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test/mock files were failed to compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strange, because there is one more below:
impl<AccountId, HoldReason> Currency<AccountId> for NoCurrency<AccountId, HoldReason> {
type Balance = u128;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted and it works now.
The PR aligns types between Bulletin and SDK. Addresses paritytech/polkadot-bulletin-chain#86 Relates to * paritytech/polkadot-bulletin-chain#128 * paritytech/polkadot-bulletin-chain#134 * #10582 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Branislav Kontur <[email protected]>
The PR aligns types between Bulletin and SDK. Addresses paritytech/polkadot-bulletin-chain#86 Relates to * paritytech/polkadot-bulletin-chain#128 * paritytech/polkadot-bulletin-chain#134 * #10582 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Branislav Kontur <[email protected]> (cherry picked from commit 18cb5ee)
Addresses #86