Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

IPC-361: Pre-release and minor fixes #271

Merged
merged 10 commits into from
Nov 2, 2023
Merged

IPC-361: Pre-release and minor fixes #271

merged 10 commits into from
Nov 2, 2023

Conversation

adlrocha
Copy link
Contributor

@adlrocha adlrocha commented Nov 2, 2023

This PR addresses the following issue raised by @aakoshh: consensus-shipyard/ipc#361 (review)

  • It adds a pre-release method to the subnet contract to allow users to recover their initial balance if they need to before the subnet bootstraps.
  • It fixes a bug in leave and allows validators leaving the subnet before it is bootstrapped to recover their initial balance.

@adlrocha adlrocha changed the title Ipc 361 pre release IPC-361: Pre-release and minor fixes Nov 2, 2023
@@ -438,7 +440,6 @@ library LibStaking {
s.validatorSet.confirmWithdraw(validator, amount);

// release stake from gateway and transfer to user
IGateway(s.ipcGatewayAddr).releaseStake(amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's see if I understand this: the stake is added to the gateway in one go when the subnet registers itself, right? So it makes sense that before that, this should not be trying to release anything.

Strange we haven't caught any issues with this 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know. I feel that there were a bunch of tests that were commented so we lost track of this. But I included a few to catch it. Good that you raised this issue, as it already uncovered this bug :)

revert NotEnoughBalance();
}

s.genesisBalance[msg.sender] -= amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this can't go negative, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It reverts on underflow/overflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are uint256, they can't be negative (they could theoretically underflow though). However, if preFund and preRelease don't have bugs this shouldn't happen.

In spite of this, do you think we should add a sanity-check just to be safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay, I recommended the balance < amount => revert fix, but if it would revert even if you try an underflow, then it's okay, what matters is there is no way get more out.

…pc-solidity-actors into ipc-361-pre-release

Signed-off-by: Alfonso de la Rocha <adlrocha@tutamail.com>
Signed-off-by: Alfonso de la Rocha <adlrocha@tutamail.com>
Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

LGTM!

@adlrocha adlrocha merged commit b4c23f2 into dev Nov 2, 2023
6 checks passed
@adlrocha adlrocha deleted the ipc-361-pre-release branch November 2, 2023 16:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants