Skip to content

feat!: staking escrow#14339

Merged
LHerskind merged 11 commits into
nextfrom
lh/13870
May 21, 2025
Merged

feat!: staking escrow#14339
LHerskind merged 11 commits into
nextfrom
lh/13870

Conversation

@LHerskind

@LHerskind LHerskind commented May 15, 2025

Copy link
Copy Markdown
Contributor

Fixes #13870 and the staking portion of #13871.

Starts the work for the staking escrow (not fully tested but more tests also needed along with the governance part of it, so in a decent spot).

Notable changes:

  • as a shared contract there is now the GSE that is escrowing funds staked, and support moving it along.
    • it is another contract that must be called when adding a new rollup if it is to move along, since it is too dangerous to just move it along in the hopes that the rollup will work similar to the old one.
  • starts the work to have different values for deposit and staying staked, but atm they are set to the same value to not make this pr even bigger than it already is
  • deployments in ts are passing along a full "operator" or validator, e.g., the set of addresses instead of just a single address and using that for all.
  • updates the staking handler to by default follow the canonical.

LHerskind commented May 15, 2025

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@LHerskind LHerskind marked this pull request as ready for review May 15, 2025 14:59
@LHerskind LHerskind changed the title chore: cleanup stakinglib usage slightly feat!: staking escrow May 15, 2025

@just-mitch just-mitch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice. Minor comments.

vm.prank(SLASHER);
staking.slash(ATTESTER, MINIMUM_STAKE / 2);

assertEq(stakingAsset.balanceOf(address(staking)), MINIMUM_STAKE);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would maybe be good to show that the gse has 0 balance here


vm.prank(SLASHER);
staking.slash(ATTESTER, depositAmount / 2);
staking.slash(ATTESTER, MINIMUM_STAKE / 2);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

good to show the LIVING status here I think

rollupAddress: Hex,
stakingAssetAddress: Hex,
validators: Hex[],
validators: Operator[],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

unrelated to this PR, but we can probably remove the cheat_ from this function name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will rename it to addMultipleValidators it is only used in here directly, so seems like a neat tiny fix.

error Staking__NotWithdrawer(address, address); // 0x8e668e5d
error Staking__NothingToExit(address); // 0xd2aac9b6
error Staking__WithdrawalNotUnlockedYet(Timestamp, Timestamp); // 0x88e1826c
error Staking__WithdrawFailed(address);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
error Staking__WithdrawFailed(address);
error Staking__WithdrawFailed(address); // 0x377422c1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

At this point the number of errors here and there are kinda pain to update. Might add something to the bootstrap so we just spit a file out that have all the errors and event codes and such to make it simpler to keep up to date, pretty sure that some of the hexes here and there are broken

canonical.push(block.timestamp.toUint32(), uint224(uint160(_rollup)));
}

function deposit(address _attester, address _proposer, address _withdrawer, bool _onCanonical)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

might explain how you can serve as an attester on multiple versions?

Comment thread l1-contracts/src/core/staking/GSE.sol Outdated

STAKING_ASSET.transfer(msg.sender, amountWithdrawn);

return (amountWithdrawn, removed);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All good, but the semantics/names are a little confusing when the amount reduces below the min balance, because the "amountWithdrawn" doesn't reflect the slashed amount.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The function is not doing any slashing, but just withdrawing, which can be used as part of slashing, but the rollup needs to deal with the logic itself. Have extended the natspec comment.

This was linked to issues May 20, 2025
Base automatically changed from lh/nuke-cheaters to master May 20, 2025 10:04
@github-actions

github-actions Bot commented May 20, 2025

Copy link
Copy Markdown
Contributor

Docs Preview

You can check your preview here.

@LHerskind LHerskind changed the base branch from master to next May 20, 2025 14:05
@LHerskind LHerskind changed the base branch from next to graphite-base/14339 May 20, 2025 14:11
@LHerskind LHerskind changed the base branch from graphite-base/14339 to master May 20, 2025 14:11
@LHerskind LHerskind requested a review from charlielye as a code owner May 20, 2025 16:08
@LHerskind LHerskind changed the base branch from master to graphite-base/14339 May 20, 2025 17:11
@LHerskind LHerskind force-pushed the graphite-base/14339 branch from c7229db to c58ae65 Compare May 20, 2025 17:11
@LHerskind LHerskind changed the base branch from graphite-base/14339 to next May 20, 2025 17:11
}),
});

// Hand over the registry to the governance

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// Hand over the registry to the governance
// Hand over the GSE to the governance

await Promise.all(txHashes.map(txHash => extendedClient.waitForTransactionReceipt({ hash: txHash })));
};

export type Operator = {

@Maddiaa0 Maddiaa0 May 21, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Observational nit here would be that Operator carries multiple meanings in realtion to ATPs and elsewhere, this could maybe be RollupOperator or ValidatorTuple or something (no change required just a comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Might make sense to deal with as part of #14287.

Comment thread l1-contracts/src/core/staking/GSE.sol Outdated
indices[i] = i;
}

return _getAddressFromIndecesAtTimestamp(_instance, indices, _timestamp);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
return _getAddressFromIndecesAtTimestamp(_instance, indices, _timestamp);
return _getAddressFromIndicesAtTimestamp(_instance, indices, _timestamp);

same elsewhere

override(IGSE)
returns (address[] memory)
{
// @todo Throw me in jail for this crime against humanity

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

brother

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I stand by the statement.

* @dev Must be the current canonical for `_onCanonical = true` to be valid.
*/
function deposit(address _attester, address _proposer, address _withdrawer, bool _onCanonical)
external

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i guess we now dont allow depositing more than the mimumim if you feel like it. It also is not clear how it can be topped up

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Atm no topup, same as it was before.

@LHerskind LHerskind added this pull request to the merge queue May 21, 2025
github-merge-queue Bot pushed a commit that referenced this pull request May 21, 2025
Fixes #13870 and the staking portion of #13871.

Starts the work for the staking escrow (not fully tested but more tests
also needed along with the governance part of it, so in a decent spot).

Notable changes:

- as a shared contract there is now the GSE that is escrowing funds
staked, and support moving it along.
- it is another contract that must be called when adding a new rollup if
it is to move along, since it is too dangerous to just move it along in
the hopes that the rollup will work similar to the old one.
- starts the work to have different values for deposit and staying
staked, but atm they are set to the same value to not make this pr even
bigger than it already is
  - see #14304  
- deployments in ts are passing along a full "operator" or validator,
e.g., the set of addresses instead of just a single address and using
that for all.
- updates the staking handler to by default follow the canonical.
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 21, 2025
@LHerskind LHerskind added this pull request to the merge queue May 21, 2025
Merged via the queue into next with commit 7a96129 May 21, 2025
6 checks passed
@LHerskind LHerskind deleted the lh/13870 branch May 21, 2025 14:44
Thunkar pushed a commit that referenced this pull request May 23, 2025
Fixes #13870 and the staking portion of #13871.

Starts the work for the staking escrow (not fully tested but more tests
also needed along with the governance part of it, so in a decent spot).

Notable changes:

- as a shared contract there is now the GSE that is escrowing funds
staked, and support moving it along.
- it is another contract that must be called when adding a new rollup if
it is to move along, since it is too dangerous to just move it along in
the hopes that the rollup will work similar to the old one.
- starts the work to have different values for deposit and staying
staked, but atm they are set to the same value to not make this pr even
bigger than it already is
  - see #14304  
- deployments in ts are passing along a full "operator" or validator,
e.g., the set of addresses instead of just a single address and using
that for all.
- updates the staking handler to by default follow the canonical.
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.

feat(GSE/Rollup): Use GSE in rollup feat(GSE): staking escrow

3 participants