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

2368 stake table registration with fixed stake #2365

Merged
merged 9 commits into from
Dec 6, 2024

Conversation

alysiahuggins
Copy link
Contributor

@alysiahuggins alysiahuggins commented Dec 5, 2024

Closes #2368 and is a sub issue of #2304
Only the registration and withdraw function should be reviewed for the changes mentioned below.

This PR:

  • adds a fixed stake amount
  • adds more verification to the register and withdraw functions
  • updated related tests

Key places to review:

  • register function
  • updated data type for the stake amount from uint64 to uint256

How to test this PR:

  • forge test --match-contract StakeTable

Things tested

  • verification logic such as invalid token balance, invalid token amount, invalid bls key

TODO

  • add tests for the withdraw function

@alysiahuggins alysiahuggins changed the title 2304 stake table registration 2304 stake table registration with fixed stake Dec 6, 2024
@alysiahuggins alysiahuggins changed the title 2304 stake table registration with fixed stake 2368 stake table registration with fixed stake Dec 6, 2024
…ions that need to be updated to the new spec
Copy link
Member

@zacshowa zacshowa left a comment

Choose a reason for hiding this comment

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

I do think tests should be added for the withdrawl functionality as that is rather important for users staking. The implementation looks fine to me, but I think we should prove it :)
Unless that is out of scope for this PR of course.

@alysiahuggins
Copy link
Contributor Author

thanks for @zacshowa , i didn't add tests for it as it's out of scope for this PR but created an issue for it #2372

Copy link
Member

@zacshowa zacshowa left a comment

Choose a reason for hiding this comment

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

LGTM

@alysiahuggins alysiahuggins merged commit 713558d into main Dec 6, 2024
22 checks passed
@alysiahuggins alysiahuggins deleted the 2304-stake-table-registration branch December 6, 2024 18:51
tbro pushed a commit that referenced this pull request Dec 6, 2024
* add stake table tests

* remove stake types

* verify token allowance, balance and reprioritize verification order on registration

* set the fixed stake amount, added related tests, updated data types

* add more verification checks to the withdraw function

* updated errror types

* added TODO statements in comments to be explicit about outdated functions that need to be updated to the new spec
tbro pushed a commit that referenced this pull request Dec 6, 2024
* add stake table tests

* remove stake types

* verify token allowance, balance and reprioritize verification order on registration

* set the fixed stake amount, added related tests, updated data types

* add more verification checks to the withdraw function

* updated errror types

* added TODO statements in comments to be explicit about outdated functions that need to be updated to the new spec
sveitser added a commit that referenced this pull request Dec 10, 2024
* Initial draft for minimal L1 stake table

* Add Schnorr verifying key

* fix solhint error: named mapping keys and values

* WIP: contract stake table types to rust types

Missing conversion from G2 Affine to BLSPubKey.

* rename: SimpleStakeTable -> PermissionedStakeTable

* PermissionedStakeTable contract: add DA flag

- Update bindings.
- Add tests for event emission.

* Allow setting an initial stake table on deployment

- The stake table isn't useful without stakers I think it makes sense to
  require it to be provided on deployment.

* deploy: load initial stake table from toml file

* Use serde instead of unsafe rust

We will add some more ergonomic code for conversion to jellyfish at some
point. This code can be used as a stop gap until then.

* Update utils/src/deployer.rs

Co-authored-by: Alysia Tech <[email protected]>

* WIP: load initial stake table from file

* Move solidity <-> jf code to contract adapter

* Load initial stake table from file

* remove unused imports

* remove unused module

* dev-node: fix arguments to deploy function

* fix clippy

* fix Cargo.toml for bindings generation

breaks if we use: `serde.workspace = true`

* combine adding and removing into single event

* update comment to match implementation (#2282)

* align comment with implementation

* added more clarity

* comment format

* clearer comment

* update comment to make it clear that there may be outdated elements in the state history

* update query-service

* db max connections = 25 for dev node tests

* update query-service

* Fix no-storage decides

* Branches -> tags

* 2368 stake table registration with fixed stake (#2365)

* add stake table tests

* remove stake types

* verify token allowance, balance and reprioritize verification order on registration

* set the fixed stake amount, added related tests, updated data types

* add more verification checks to the withdraw function

* updated errror types

* added TODO statements in comments to be explicit about outdated functions that need to be updated to the new spec

* fix env var for permissioned_stake_table

* Add test for jf / contract types conversions

* cleanup: pass rng

* load stake table from toml file (#2377)

* change lc proxy addr

* fix deploy-sequencer-contracts in docker compose

* test for stake table from toml file

* initial stake table toml file

* exclude toml file in typos

* remove custom deser

* remove unnecessary rand dependency

---------

Co-authored-by: Alysia Tech <[email protected]>
Co-authored-by: tbro <[email protected]>
Co-authored-by: imabdulbasit <[email protected]>
Co-authored-by: Artemii Gerasimovich <[email protected]>
Co-authored-by: Abdul Basit <[email protected]>
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.

Stake Table: modify the registration function to only accept a fixed stake
2 participants