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

StakingV4: Create auction list selector subcomponent #4073

Merged

Conversation

mariusmihaic
Copy link
Contributor

@mariusmihaic mariusmihaic commented May 12, 2022

This is mostly a refactor. One could check the changes by using a tool to compare systemSCs.go with auctionListSelector.go, as well as their test files.

What's new

  • Created MaxNodesChangeConfigProvider interface which provides the current max nodes config based on the current epoch. This was done because the same logic is implemented in several places: systemSC, auctionListSelector and randHashedShuffler. DID NOT integrate this interface in randHashedShuffler yet, since that part would require too much of a refactor. Created a separate branch for it: nodes-config-provider-into-shuffler
  • Fixed possible div by zero in auction list selector by simply checking if len(randomness) == 0

Refactor

  • Created AuctionListSelector interface, which handles nodes selection from auction list
  • Moved all code from systemSCs.go (as well as any related tests) to auctionListSelector.go/auctionListSelector_test.go
  • Integrated AuctionListSelector & MaxNodesChangeConfigProvider in SystemSC.

@mariusmihaic mariusmihaic changed the title FEAT: First ugly version Create auction list selector subcomponent May 12, 2022
@mariusmihaic mariusmihaic self-assigned this May 13, 2022
@mariusmihaic mariusmihaic changed the title Create auction list selector subcomponent StakingV4: Create auction list selector subcomponent May 13, 2022
@mariusmihaic mariusmihaic marked this pull request as ready for review May 17, 2022 08:23
@sstanculeanu sstanculeanu self-requested a review May 17, 2022 11:19
sstanculeanu
sstanculeanu previously approved these changes May 17, 2022
@bogdan-rosianu bogdan-rosianu self-requested a review May 17, 2022 13:01
epochStart/errors.go Show resolved Hide resolved
nodesConfigProvider epochStart.MaxNodesChangeConfigProvider
}

// AuctionListSelectorArgs is a struct placeholder for all arguments required to create a NewAuctionListSelector
Copy link
Contributor

Choose a reason for hiding this comment

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

to create an auctionListSelector*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both are linked by Goland editor, is there a reason to use auctionListSelector instead of NewAuctionListSelector ?

Copy link
Contributor

Choose a reason for hiding this comment

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

you don't create a NewAuctionListSelector.. but rather the argument struct is used to create an auctionListSelector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, will fix it in the upcoming PR

epochStart/metachain/auctionListSelector.go Outdated Show resolved Hide resolved
epochStart/metachain/auctionListSelector.go Show resolved Hide resolved
epochStart/metachain/auctionListSelector.go Show resolved Hide resolved
"num of nodes which will be shuffled out", numOfShuffledNodes,
"num of validators after shuffling", numOfValidatorsAfterShuffling,
"auction list size", auctionListSize,
fmt.Sprintf("available slots (%v -%v)", maxNumNodes, numOfValidatorsAfterShuffling), availableSlots,
Copy link
Contributor

Choose a reason for hiding this comment

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

%v - %v*
also, log debug maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

%v - %v*

Any particular reason for this suggestion? I like the current version better

Copy link
Contributor

Choose a reason for hiding this comment

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

Because available slots (3 - 5) is better than available slots (3 -5).
Also, why don't you use %d instead of %v ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because available slots (3 - 5) is better than available slots (3 -5)

Right, I thought you suggested to remove the paranthesis

Also, why don't you use %d instead of %v ?

%v is the default value in golang, and for decimals it translates to %d, so it should make no difference, right?

epochStart/metachain/auctionListSelector.go Show resolved Hide resolved
return ret, nil
}

func calcNormRand(randomness []byte, expectedLen int) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use full words?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to calcNormalizedRandomness

epochStart/metachain/auctionListSelector.go Show resolved Hide resolved
epochStart/notifier/nodesConfigProvider.go Show resolved Hide resolved
Base automatically changed from EN-11959-staking-v4-integration-tests-custom-scenarios to feat/liquid-staking May 23, 2022 07:39
@mariusmihaic mariusmihaic merged commit b4bb1d0 into feat/liquid-staking May 23, 2022
@mariusmihaic mariusmihaic deleted the EN-12187-auction-list-selection-subcomp branch May 23, 2022 11:48
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.

None yet

3 participants