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

Implement delegator slot reservation #4841

Open
wants to merge 45 commits into
base: feat-2.0
Choose a base branch
from

Conversation

wojcik91
Copy link
Collaborator

No description provided.

@wojcik91
Copy link
Collaborator Author

@mpapierski I did implement the solution we've discussed to the issue of handling both legacy and updated SeigniorageRecipientsSnapshot:

  • added a named key to the auction contract to store the version flag
  • check for presence of flag when reading the snapshot for arbitrary era

I've also added a method to add this new named key and migrate current snapshot to new version as part of protocol upgrade, but this is a part of the codebase I'm unfamiliar with, so I'm not sure if this is the correct approach.
I know my changes introduced some test failures, but before I delve into debugging those please let me know if this is a valid solution or if I should approach the topic differently.

Copy link
Collaborator

@mpapierski mpapierski left a comment

Choose a reason for hiding this comment

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

Looks solid as implemented, although you should take into account an optional migration of named keys etc which would not happen at 2.0.0 release.

storage/src/system/auction.rs Outdated Show resolved Hide resolved
@wojcik91 wojcik91 marked this pull request as ready for review October 15, 2024 09:15
@wojcik91
Copy link
Collaborator Author

@mpapierski I implemented the fixes we've discussed + some other stuff I found. I also added a migration test.

@EdHastingsCasperAssociation
Copy link
Collaborator

@darthsiroftardis can you review this, paying particular attention to entity migration on / off integration

@darthsiroftardis
Copy link
Contributor

darthsiroftardis commented Oct 18, 2024

@EdHastingsCasperAssociation Looks good to me, from the entity on/off side, the only impact is when auction data from its named keys, but its using entity addr, and the changes on my end are such that the handling of the on/off bit happens within the get named keys function, so overall nothing I saw raised a flag

I would normally just approve the PR, but, I know we are still iterating over the name

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.

5 participants