rusk-wallet: Allow withdrawing of partial rewards

[Daksh14]

Pass reward param to withdraw transaction function Add check for withdraw amount

Closes #2538

[moCello]

Some comments, the biggest one being that I would have the stake-reward be an optional value. So that when the user doesn't specify anything, the entire reward is withdrawn.

[moCello]

I would not add short here as that option has caused clashes in the past and I see no problem with only allowing the stake-reward being passed as a long argument.

[Daksh14]

I see no problem in adding short. Those clashes happen if arguments have same starting alphabet, here -r or --reward shouldn't clash with anything. I'd like to keep it like all other arguments

[moCello]

I think the default option should always only be long unless there is a good reason to add it as short as well.

[moCello]

I think it makes sense to have the reward be an optional value. If None is given, we withdraw the entire reward.

[Daksh14]

I disagree, if the person isnt aware they can specify the amount and they dont want to withdraw all of it then it will be a problem, will leave the user confused. Even if we document this behavior people arent gonna read it. I can add option to withdraw all but they will have to specify if they want to do so

[moCello]

I would say that it's most likely that the user wants to withdraw all the funds so having this behavior activated by default makes sense to me.
And if not all funds should be withdrawn, then the user can specify so we don't sacrifice any functionality with my proposed solution.
We do however add friction with your suggestion since the user now needs to be aware and check the max funds available whenever she wants to retrieve the reward.

The case that the user is not aware of certain functionalities shouldn't guide our design decisions.

[HDauven]

@Daksh14 I agree with Moana here. The approach she suggests would lead to the least amount of friction. Users will always have to end up requesting for their reward balance, even though the vast majority will want to withdraw their full reward balance anyway.

We should always design with Pareto principle in mind. Focus on the needs of the majority (80%), but make sure the minority (20%) can still achieve their goals

[Daksh14]

Amt is now optional and if its none then all the available rewards will be withdrawn @HDauven @moCello

[moCello]

Beware of the naming of functions. The stake-amount is not the same as the stake-reward.

I also don't think you really need this function, but that it makes more sense to use the get_stake_info method that is already implemented.

[Daksh14]

I need the helper function because get_stake_info is a method of the State which wallet keeps track off. I'll rename the function to get_stake_reward

[moCello]

but get_stake_amount is also a method of the state. Indeed the first three lines of get_stake_amount can be replaces by self.stake_info(sender_index)

Mapping that output to one field of the stake-info doesn't justify an extra method to me.

[moCello]

and my appologies, I meant stake_info not get_stake_info in my first comment.

[Daksh14]

please check the contents of get_stake_reward function. It requires a helper function because we need to access the state that means we have to be connected to the chain and be online which can be only accessed through the Wallet struct so it needs to be a method @moCello

[moCello]

This is the function I'm referring to, in line 466 of the same file:

    /// Obtains stake information for a given address.
    pub async fn stake_info(
        &self,
        profile_idx: u8,
    ) -> Result<Option<StakeData>, Error> {
        self.state()?
            .fetch_stake(self.public_key(profile_idx)?)
            .await
    }

Afaics it does the same as the first lines of your proposed new function.

Accessing one field of the obtained stake-info, to me doesn't justify a new method on the wallet.

[Daksh14]

I changed the method to use the stake_info method now double check and let me know @moCello