Permissioned L1 stake table #2325
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sveitser
changed the title
Missing conversion from G2 Affine to BLSPubKey.
- Update bindings. - Add tests for event emission.
- The stake table isn't useful without stakers I think it makes sense to require it to be provided on deployment.
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.
Co-authored-by: Alysia Tech <[email protected]>
breaks if we use: `serde.workspace = true`
3 tasks
* 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
* 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
imabdulbasit
reviewed
Dec 9, 2024
imabdulbasit
reviewed
Dec 9, 2024
* 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
sveitser
requested review from
philippecamacho,
ImJeremyHe and
jbearer
as code owners
alysiahuggins
approved these changes
Dec 10, 2024
There was a problem hiding this comment.
Approving so that work can progress but a question around event logs.
So theres's an event when the stakers list is updated but not an event when one is inserted or removed.
Is that because inserts only happen at the beginning when the existing list of stakers are added to the contract?
Though I presume a removal can only happen in the future.
Assuming there's a service that depends entirely on this contract to know the full active set of stakers, shouldn't there be emit logs for those methods too?
EDITED
only the update is public so nevermind
sveitser
commented
Dec 10, 2024
|
|
||
| // public methods | ||
|
|
||
| function update(NodeInfo[] memory stakersToRemove, NodeInfo[] memory newStakers) |
There was a problem hiding this comment.
@alysiahuggins only the update method is public.
There was a problem hiding this comment.
ah missed that, thanks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some info: https://www.notion.so/espressosys/Permissioned-stake-table-contract-for-Dec-2024-release-1462431b68e980a0a4abcd6bb65d7e43?pvs=4
TODO
NodeInfo.Follow up work