Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Factor out checks for staking's bond & nominate #10751

Closed
wants to merge 2 commits into from

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Jan 29, 2022

This is a back port from #10694.

The new functions factored out here will be used to implement a new PoolsInterface:

fn bond_checks(
stash: &Self::AccountId,
controller: &Self::AccountId,
value: Self::Balance,
_: &Self::AccountId,
) -> Result<(), DispatchError> {
Self::do_bond_checks(stash, controller, value)?;
if frame_system::Pallet::<T>::can_inc_consumer(stash) {
Ok(())
} else {
Err(Error::<T>::BadState.into())
}
}
fn bond(
stash: Self::AccountId,
controller: Self::AccountId,
value: Self::Balance,
payee: Self::AccountId,
) -> DispatchResult {
Self::bond(
RawOrigin::Signed(stash).into(),
T::Lookup::unlookup(controller),
value,
RewardDestination::Account(payee),
)
}
fn nominate_checks(
controller: &Self::AccountId,
targets: Vec<Self::LookupSource>,
) -> Result<(Self::AccountId, Vec<Self::AccountId>), DispatchError> {
Self::do_nominate_checks(controller, targets)
}
fn unchecked_nominate(stash: &Self::AccountId, targets: Vec<Self::AccountId>) {
Self::do_unchecked_nominate_writes(stash, targets);
}

The idea is these checks can be exposed by trait PoolsInterface. Additionally, since the nominate checks map & collect the targets, we avoid redundant memory usage and computation by piping the targets from the check into a write function. The plan is for this nominate write function to also be exposed by the PoolsInterface so the pool can call the check, do some other checks, and then input the targets into the write function.

I still not sure this is the right approach as it makes the PoolsInterface a bit more leaky, so this needs some discussion.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jan 29, 2022
@emostov emostov added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jan 29, 2022
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

should we prefer ensure prefix to be more aligned with the rest of the codebase?

) {
let nominations = Nominations {
targets,
// Initial nominations are considered submitted at era 0. See `Nominations` doc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Initial nominations are considered submitted at era 0. See `Nominations` doc.
// Initial nominations are considered submitted at era 0. See [`Nomination`]'s doc.

@kianenigma
Copy link
Contributor

looking at your other branch, I am not sure either if this is the right approach, so perhaps not backporting this yet is a good idea.

Fundamentally, why do you need these ensure functions to be exposed? Why can't you just call StakingInterface::nominate, which returns a Result? (at it is already doing, aka. DispatchResult).

@emostov
Copy link
Contributor Author

emostov commented Jan 31, 2022

As per discussed offline, I am going to look into using transactional TXs instead of added can_/ensure_ type functions, so closing for now

@emostov emostov closed this Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants