Skip to content
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

stake-contract: allow stake topup and partial unstake #3027

Open
wants to merge 12 commits into
base: remove-stake-nonce
Choose a base branch
from

Conversation

herr-seppia
Copy link
Member

No description provided.

@herr-seppia herr-seppia changed the title wip stake-contract: allow stake topup and partial unstake Nov 25, 2024
@herr-seppia herr-seppia marked this pull request as ready for review November 25, 2024 12:05
Comment on lines +141 to +142
/// Return None if the entry was not found or `amount` is higher than
/// current stake
Copy link
Member

Choose a reason for hiding this comment

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

Is it smart to have the same return value for a non-existing stake and overflowing subtraction? I would think returning a Result might make sense here, but I don't have a good overview on how and where this function will be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not smart I agree
However, I thought that introducing an error for distinguish the cases was a bit overkilling for our need.

Indeed both cases (entry not found or amount higher), identify a bug in our code and the remediation is to resync the provisioners from scratch asking them to the stake contract with

            if let Err(e) = selective_update {
                warn!("Resync provisioners due to {e:?}");
                let state_hash = blk.header().state_hash;
                let new_prov = vm.get_provisioners(state_hash)?;
                provisioners_list.update_and_swap(new_prov)
            }

Having the "bug" logger somewhere is enough IMO


/// Add an amount to the stake
pub fn add(&mut self, add: u64) {
self.value += add
Copy link
Member

@moCello moCello Nov 25, 2024

Choose a reason for hiding this comment

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

overflow check? Or do we assume that no-one can ever own that much

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the value is the lux representation of the stake, someone should own more than 18B DUSK in order to overflow (way more than our max supply)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants