NDRS-215: Unify bids persistence by auction contract by mpapierski · Pull Request #165 · casper-network/casper-node

mpapierski · 2020-08-24T14:50:10Z

Unifies storage of bids and founding validators into one collection where founding/non-founding validators are determined upon an attribute is_founding_validator

grpc/tests/src/test/system_contracts/auction/bids.rs

afck · 2020-08-28T08:28:50Z

grpc/tests/src/test/system_contracts/auction/bids.rs

+
+    builder.exec(transfer_request_1).commit().expect_success();
+
+    //


No comment. 😄

afck · 2020-08-28T08:41:19Z

smart_contracts/contracts/system/auction/src/lib.rs

    let mut entry_points = EntryPoints::new();

+    let entry_point = EntryPoint::new(
+        METHOD_RELEASE_FOUNDER,


Should this really be an entry point of the auction contract? Who is going to call that?

I was thinking they would automatically released in the run_auction call with the corresponding era number?

Doable - I actually have ticket this sprint to refine this https://casperlabs.atlassian.net/browse/NDRS-231

OK. I guess we'll never actually use this entry point then. Would probably be more work to call it from BlockExecutor than to implement NDRS-231.

Yes - I plan to remove this entry point since I'll be introducing a field to indicate which era it will unlock, there's no need for a special entry point

afck · 2020-08-28T08:42:57Z

types/src/auction/args.rs

-pub const FOUNDING_VALIDATORS_KEY: &str = "founding_validators";
-/// Storage for `ActiveBids`.
-pub const ACTIVE_BIDS_KEY: &str = "active_bids";
+pub const VALIDATORS_KEY: &str = "validators";


Isn't "active bids" (or just "bids") a better term? Because they are not all validators, only the winner are?

Yep fair I changed the data structure to Bid and collection of bids to Bids and the relevant keys.

afck · 2020-08-28T08:45:29Z

types/src/auction/auction_provider.rs

-    /// For a founding validator, the same logic is carried out with founding_validators, instead
-    /// of `active_bids`.
+    /// For a founding validator, the same logic is carried out with validators, instead
+    /// of `founders`.


Is that comment still accurate? Now both cases seem to access the validators map.

afck · 2020-08-28T08:49:20Z

types/src/auction/auction_provider.rs

-        // Return early if target validator is not in `active_bids`
-        let _active_bid = active_bids
+        // Return early if target validator is not in `validators`
+        let _founding_validator = validators


Same as above: This is not a founding validator, is it?

afck · 2020-08-28T08:49:41Z

types/src/auction/auction_provider.rs

+        //     .iter()
+        //     .map(|(validator_account_hash, active_bid)| {
+        //         (*validator_account_hash, active_bid.bid_amount)
+        //     });


Is that comment left by mistake?

afck · 2020-08-28T08:50:46Z

types/src/auction/founding_validator.rs

            staked_amount,
            delegation_rate: 0,
            funds_locked: true,
+            is_founding_validator: true,


I guess that should be:

Suggested change

is_founding_validator: true,

is_founding_validator: false,

afck · 2020-08-28T08:51:01Z

types/src/auction/founding_validator.rs

+        }
+    }
+
+    /// Creates a new instance of `FoundingValidator` for non-founding entry.


Suggested change

/// Creates a new instance of `FoundingValidator` for non-founding entry.

/// Creates a new instance of `Validator` for non-founding entry.

…or ActiveBid or Bid?

afck · 2020-08-28T08:52:02Z

types/src/auction/founding_validator.rs

+            !self.funds_locked
+        } else {
+            // Non founding validator can always withdraw funds
+            true


pub fn can_withdraw_funds(&self) -> bool { !self.is_founding_validator || !self.funds_locked }

But actually: Why do we need to distinguish founding validators at all? Is there any other reason why your funds could be locked?

So founding validator's should have funds unlocked at certain era, right? Non-founding entries aren't locked

Exactly… why does the is_founding_validator boolean exist at all?
Should this method maybe just return !self.funds_locked and that's it?

As discussed on slack I removed the bool flag

Bump to 1.5.8

mpapierski requested review from AlexanderLimonov, EdHastingsCasperAssociation, Fraser999 and afck August 24, 2020 14:50

mpapierski assigned EdHastingsCasperAssociation Aug 24, 2020

mpapierski force-pushed the ndrs-215-unify-storage branch from 3209c32 to 4b91dbb Compare August 26, 2020 16:03

NDRS-215: Unify storage of bids.

528756e

mpapierski force-pushed the ndrs-215-unify-storage branch from 4b91dbb to 528756e Compare August 27, 2020 10:57

mpapierski marked this pull request as ready for review August 27, 2020 10:59

EdHastingsCasperAssociation changed the title ~~NDRS-215: Unify storage~~ NDRS-215: Unify bids persistence by auction contract Aug 27, 2020

EdHastingsCasperAssociation approved these changes Aug 27, 2020

View reviewed changes

Merge branch 'master' into ndrs-215-unify-storage

34cc395

afck suggested changes Aug 28, 2020

View reviewed changes

NDRS-215: Apply round of review comments.

917e8e3

afck approved these changes Aug 28, 2020

View reviewed changes

mpapierski merged commit b9bb305 into casper-network:master Aug 28, 2020

sacherjj pushed a commit that referenced this pull request Aug 13, 2024

Merge pull request #165 from casper-network/version-bump-1.5.8

6b209d1

Bump to 1.5.8

rafal-ch pushed a commit that referenced this pull request Sep 11, 2024

Bump to v0.16.0 (#165)

a8f4fc7


		builder.exec(transfer_request_1).commit().expect_success();

		//

	/// Creates a new instance of `FoundingValidator` for non-founding entry.
	/// Creates a new instance of `Validator` for non-founding entry.

Conversation

mpapierski commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afck Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mpapierski commented Aug 24, 2020 •

edited

Loading

afck Aug 28, 2020 •

edited

Loading