Add identity table smart contract #2

joshuahannan · 2020-08-03T21:16:30Z

Adding the first draft of the Flow Identity table smart contract for epochs. It contains the minimum functionality for managing the Flow identity table:

Struct definition for a node that holds its information.
- are the field names and types correct?
Dictionary for holding all nodes' information, indexed by ID.
- Is this the best way to store and index the info?
Restriction for node initialization.
- need help with some of the pre-conditions
- Should we try to enforce that some of the values for certain fields are not used by multiple nodes?
Admin resource for updating the table.
Public functions to read info about the nodes. (Do we want all of this info to be publicly accessible?)

I'll start writing tests for it once I get sign-off from the various stakeholders.

Closes https://github.com/dapperlabs/flow-go/issues/4531

turbolent

Looks good! Just some minor suggestions

contracts/epochs/FlowIdentityTable.cdc

zhangchiqing

Looks good. Have a few questions.

contracts/epochs/FlowIdentityTable.cdc

jordanschalm

Nice work!

contracts/epochs/FlowIdentityTable.cdc

joshuahannan · 2020-08-05T15:20:59Z

contracts/epochs/FlowIdentityTable.cdc

+                FlowIdentityTable.nodes[id] == nil: "The ID cannot already exist in the record"
+                role >= 1 && role <= 4: "The role must be 1, 2, 3, or 4"
+                networkingAddress.length > 0: "The networkingAddress cannot be empty" // TODO: Require exact length
+                initialWeight > UInt64(0): "The initial weight must be greater than zero" // TODO: Max weight


@jordanschalm is there a max initialWeight that we can enforce here?

We haven't really defined a maximum value or range for weights, but maybe something that guarantees that sum(node.initialWeight) for node in identityTable doesn't overflow uint64.

joshuahannan · 2020-08-05T15:24:30Z

@jordanschalm should we enforce that some of the values for certain fields are not used by multiple nodes? For example, should we put in the code that it should revert if someone tries to create a Node struct with the a networking Address or keys that already exist in the table? Or are are those allowed to be shared?

jordanschalm · 2020-08-05T16:56:10Z

should we enforce that some of the values for certain fields are not used by multiple nodes?

Good point. Yes, we should enforce this for network addresses and public keys.

joshuahannan · 2020-08-10T19:30:05Z

I added a new draft to include the current, proposed, and previous node tables in this smart contract and I addressed all the PR comments, including changing some field Types, changing function names, adding comments, and adding pre-conditions.

Can I get another review? @turbolent @zhangchiqing @jordanschalm

@zhangchiqing I still feel that we shouldn't include the epoch id in this smart contract because that is going to be managed by the Epoch lifecycle contract, which this contract doesn't need to explicitly know about. As you can see now, updating the node table is handled through the updateCurrentNodeTable function, which just sets the current node table as the value of proposed node table. The epoch lifecycle contract will be the one to enforce that this only happens when the epoch boundary and ID change happens. Let me know if you think this is still a problem and we can discuss more.

jordanschalm

Nice work!

joshuahannan · 2020-08-11T21:49:35Z

I added sample transactions, unit tests, a templates package.

I also made some changes to store the epoch ID and put the ID tables in one field based on a conversation that @zhangchiqing and I had. Let me know if you have any questions and I can explain.

@jordanschalm and @zhangchiqing can you review one more time? Thank you!

contracts/epochs/FlowIdentityTable.cdc

zhangchiqing · 2020-08-12T01:44:40Z

contracts/epochs/FlowIdentityTable.cdc

+
+        /// update the entire node table
+        /// This will be called at the beginning of a new epoch
+        pub fun startNewEpoch() {


It's better to take the newEpochID as input, so that the FlowIdentityTable can also verify it, and ensure everything is synced.

Suggested change

pub fun startNewEpoch() {

pub fun startNewEpoch(Uint64 newEpochID) {

pre {

newEpochID == FlowIdentityTable.currentEpochID + UInt64(1): "invalid new epoch ID"

}

}

As far as I can see, giving the epoch ID as an argument is exactly the same as not doing it, so I don't believe this is necessary. The admin would still be able to call this as many times as they want either way

I see Leo's point here. There should be one source of truth for the current epoch counter among the smart contracts. If that source of truth is the EpochManager (sorry, I forgot what you actually called this josh 😅), then that contract should pass the epoch counter along to this one, rather than both determining it independently. Of course adding 1 to a counter isn't so complicated, but I agree it's better that the synchronization between the two is explicit.

Yes, the source of truth is in EpochContract. Adding the newEpochID and the pre check seems unnecessary, but it allows us to catch potential bug earlier and reason about the problem easier. So if there was a unknown bug somewhere, we could be sure that the state transaction of the identity table should be correct, because we've explicitly checked, rather than assuming EpochContract didn't make mistake of what's the current epoch the identity table was cached.

I thought we said that we wanted the identity table contract to be the source of truth for the epoch counter? I was thinking that the the Epoch Contract wouldn't have a field for that and would just get it from this contract

I thought we said that we wanted the identity table contract to be the source of truth for the epoch counter?

Oh no, that's not what I meant.

EpochContract should own the source of truth for what is the current epoch ID. The identity table has a copy of the current epoch ID. When start a new epoch, the EpochContract and IdentityTable will sync what's the current epoch ID, so that addProposedNode/removeProposedNode can trust that the the copy of the current EpochID is consistent with EpochContract's

We could also remove the trust completely by adding the epochID to addProposedNode, and adding a check. That way we are 100% sure that when proposed nodes are updated the epoch ID was consistent across IdentityTable, Admin and EpochContract:

pub fun removeProposedNode(_ proposedEpochID: UInt64, _ nodeID: String): Node? { pre { proposedEpochID == FlowIdentityTable.getProposedEpochID(): "Invalid proposedEpochID" } ... }

Ok I added those parameters to all the functions that change the state to make it consistent. 👍

contracts/epochs/FlowIdentityTable.cdc

zhangchiqing · 2020-08-12T01:52:36Z

contracts/epochs/FlowIdentityTable.cdc

+        /// Add a new node to the proposed table, or update an existing one
+        pub fun addProposedNode(_ newNode: Node) {
+            // Remove the proposed epoch table from the record
+            let proposedNodeTable = FlowIdentityTable.nodes[FlowIdentityTable.currentEpochID+UInt64(1)]!


Could we make a method for FlowIdentityTable.currentEpochID+UInt64(1)?

What kind of method? I'm not sure what you mean

I think Leo means methods for getting current/proposed tables, like getCurrentEpochTable and getProposedEpochTable

Thats already in there, below the admin resource. Or am I still misinterpreting?

contracts/epochs/FlowIdentityTable.cdc

jordanschalm · 2020-08-12T16:07:09Z

contracts/epochs/FlowIdentityTable.cdc

+        /// Add a new node to the proposed table, or update an existing one
+        pub fun addProposedNode(_ newNode: Node) {
+            // Remove the proposed epoch table from the record
+            let proposedNodeTable = FlowIdentityTable.nodes[FlowIdentityTable.currentEpochID+UInt64(1)]!


I think Leo means methods for getting current/proposed tables, like getCurrentEpochTable and getProposedEpochTable

jordanschalm · 2020-08-12T16:08:38Z

contracts/epochs/FlowIdentityTable.cdc

+
+        /// Add a new node to the proposed table, or update an existing one
+        pub fun addProposedNode(_ newNode: Node) {
+            // Remove the proposed epoch table from the record


Mainly just out of curiosity: FlowIdentityTable.nodes isn't a resource, does the value in the mapping still get "removed"? Or is it copied?

If we just used an assignment operator, then it would be copied, but since we use remove in the remove function, it actually removes it from the dictionary

contracts/epochs/FlowIdentityTable.cdc

jordanschalm · 2020-08-12T16:20:25Z

contracts/epochs/FlowIdentityTable.cdc

+
+        /// update the entire node table
+        /// This will be called at the beginning of a new epoch
+        pub fun startNewEpoch() {


I see Leo's point here. There should be one source of truth for the current epoch counter among the smart contracts. If that source of truth is the EpochManager (sorry, I forgot what you actually called this josh 😅), then that contract should pass the epoch counter along to this one, rather than both determining it independently. Of course adding 1 to a counter isn't so complicated, but I agree it's better that the synchronization between the two is explicit.

zhangchiqing · 2020-08-12T17:29:18Z

contracts/epochs/FlowIdentityTable.cdc

+            proposedNodeTable[newNode.id] = newNode
+
+            // Save the proposed epoch table back to the epoch record
+            FlowIdentityTable.nodes[FlowIdentityTable.currentEpochID+UInt64(1)] = proposedNodeTable


Does proposedNodeTable[newNode.id] = newNode already updated the record in place?

If not, does cadence allow you to do this without making a local copy?

FlowIdentityTable.nodes[FlowIdentityTable.currentEpochID+UInt64(1)]![newNode.id] = newNode

It does update the record in place, but doing it without making a local copy isn't possible in Cadence yet because of how we handle optionals. I'll speak to bastian to see if we can add a workaround

joshuahannan · 2020-08-12T17:48:24Z

contracts/epochs/FlowIdentityTable.cdc

+
+            /// The proposed nodes for the next epoch are explicitly not changed
+            /// because the proposed identity table will stay the same for the next
+            /// epoch because we assume most nodes will stay in


Ah yes good point. I'll add that.

joshuahannan · 2020-08-13T13:10:51Z

Ok I've made all the changes that were requested. Hopefully this is the last review we need @zhangchiqing @jordanschalm .

turbolent

Nice work!

turbolent · 2020-08-13T17:19:52Z

contracts/epochs/FlowIdentityTable.cdc

+        // pre {
+        //     startingEpochCounter > UInt64(0): "Must set the epoch ID as greater than zero"
+        // }


Is this commented code intentional? If it is, maybe add an additional comment why it is not enabled, or if not just remove it

oops, I was testing some things out in the init function and forgot to clean it up. Thanks for pointing that out!

turbolent · 2020-08-13T17:20:29Z

contracts/epochs/FlowIdentityTable.cdc

+        // Using this for testing. Need two admins for different contracts
+        self.account.save(<-create Admin(), to: /storage/flowIdentityTableAdmin2)
+
+        let path: Path = /storage/flowID


unused code

turbolent · 2020-08-13T17:23:17Z

contracts/epochs/FlowIdentityTable.cdc

+        /// Add a new node to the proposed table, or update an existing one
+        pub fun addProposedNode(epochCounter: UInt64, _ newNode: Node) {
+            pre {
+                epochCounter == FlowIdentityTable.currentEpochCounter + UInt64(1):


FlowIdentityTable.currentEpochCounter + UInt64(1) is used in a lot of places, maybe consider adding a function and using it instead:

fun proposedEpoch(): UInt64 { return self.currentEpochCounter + UInt64(1) }

Good suggestion! I'll add that

turbolent · 2020-08-13T17:25:38Z

transactions/idTable/create_node.cdc

+    }
+
+    execute {
+        let newNode = FlowIdentityTable.Node(id: id, role: role, networkingAddress: networkingAddress, networkingKey: networkingKey, stakingKey: stakingKey, initialWeight: initialWeight)


consider chopping this down to make it easier to read:

Suggested change

let newNode = FlowIdentityTable.Node(id: id, role: role, networkingAddress: networkingAddress, networkingKey: networkingKey, stakingKey: stakingKey, initialWeight: initialWeight)

let newNode = FlowIdentityTable.Node(

id: id,

role: role,

networkingAddress: networkingAddress,

networkingKey: networkingKey,

stakingKey: stakingKey,

initialWeight: initialWeight

)

zhangchiqing · 2020-08-14T00:02:09Z

contracts/epochs/FlowIdentityTable.cdc

+
+    /// Initialize the node record to be empty
+    init() {
+        self.currentEpochCounter = 1


How about letting EpicContract to specify the currentEpochCounter on init? And we will just check that its >= 1.

The Epoch Contract doesn't deploy this contract, we have to deploy it manually, so there is basically no difference between hardcoding it here and making it an argument, right?

zhangchiqing · 2020-08-14T00:05:34Z

contracts/epochs/FlowIdentityTable.cdc

+                    "The Epoch counter must be for the proposed Epoch"
+            }
+
+            let node = self.removeProposedNode(epochCounter: epochCounter, nodeID)


Since addProposedNode will update the node, can we read the node instead of removing the node?

No, we can't do that because the init function of a node checks that it isn't already in use, so we have to remove it, modify, and put it back.

Do you mean reading the node and updating it will trigger the init function of it? I'm not clear how it works.

zhangchiqing · 2020-08-14T00:07:22Z

contracts/epochs/FlowIdentityTable.cdc

+    pub resource Admin {
+
+        /// Add a new node to the proposed table, or update an existing one
+        pub fun addProposedNode(epochCounter: UInt64, _ newNode: Node) {


Since it allows both insert and update, consider saveProposedNode or upsertProposedNode

It actually doesn't allow updating a node any more, so I'll keep it as add, and I removed that part of the comment

Nice!

Curious how does it prevent from updating a node? Do we need a check or a test case for that?

The init function of the Node struct ensures that the node ID doesn't already exist in a pre-condition on line 57, so it would prevent it before it is even created. And I have a test case for it already. 👍

zhangchiqing

Looks good to me know. Nice work! I left some optional suggestions

zhangchiqing · 2020-08-14T14:21:51Z

contracts/epochs/FlowIdentityTable.cdc

+            FlowIdentityTable.nodes[FlowIdentityTable.proposedEpochCounter()] = FlowIdentityTable.nodes[FlowIdentityTable.currentEpochCounter]!
+
+            // Erase the records of the epoch before the previous epoch
+            FlowIdentityTable.nodes[FlowIdentityTable.currentEpochCounter-UInt64(2)] = {}


Is there a delete key word for map?

I don't think there is. @turbolent ?

zhangchiqing · 2020-08-14T14:24:38Z

contracts/epochs/FlowIdentityTable.cdc

+    pub resource Admin {
+
+        /// Add a new node to the proposed table, or update an existing one
+        pub fun addProposedNode(epochCounter: UInt64, _ newNode: Node) {


Nice!

Curious how does it prevent from updating a node? Do we need a check or a test case for that?

zhangchiqing · 2020-08-14T14:33:44Z

contracts/epochs/FlowIdentityTable.cdc

+
+        /// Update the initial weight of one of the proposed nodes
+        /// This will be called at the end of the staking auction when all
+        /// of the nodes stakes have been finally committed


Since it will only be called once at the end of the staking auction, would it make sense to allow multiple updates?

pub fun updateInitialWeight(epochCounter: UInt64, newWeights: { nodeID: UInt64 }) {

I even think we could merge updateInitWeight and startNewEpoch to be:

commitAndStartNewEpoch(newWeights: { nodeID: UInt64 }, newEpochCounter: UInt64)

So that

Less calls to the Identity Contract

Less pre checks on epochCounter

Not possible to updateInitialWeight for the same node for multiple times (currently allows)

This might be possible, but we don't update the initial weight when the staking auction ends, so it wouldn't go in the start new epoch funtion, we'd have to make a new function. I think it is fine to keep it the way it is though. I'm not concerned about it.

zhangchiqing · 2020-08-14T14:37:39Z

contracts/epochs/FlowIdentityTable.cdc

+                    "The Epoch counter must be for the proposed Epoch"
+            }
+
+            let node = self.removeProposedNode(epochCounter: epochCounter, nodeID)


Do you mean reading the node and updating it will trigger the init function of it? I'm not clear how it works.

first draft of identity table

703f23a

joshuahannan requested review from AlexHentschel, dete, jordanschalm and laynelafrance August 3, 2020 21:16

turbolent reviewed Aug 4, 2020

View reviewed changes

contracts/epochs/FlowIdentityTable.cdc Outdated Show resolved Hide resolved

contracts/epochs/FlowIdentityTable.cdc Outdated Show resolved Hide resolved

contracts/epochs/FlowIdentityTable.cdc Outdated Show resolved Hide resolved

zhangchiqing reviewed Aug 4, 2020

View reviewed changes

jordanschalm reviewed Aug 4, 2020

View reviewed changes

joshuahannan commented Aug 5, 2020

View reviewed changes

joshuahannan added 4 commits August 5, 2020 13:08

responding to PR comments

2d9a31f

added proposed and previous Fields

8fc9d63

docstrings

8081b74

more comments

f29ffcc

joshuahannan requested review from turbolent, zhangchiqing and jordanschalm August 10, 2020 19:21

comment about current Epoch

94630c6

jordanschalm approved these changes Aug 10, 2020

View reviewed changes

joshuahannan added 2 commits August 10, 2020 17:12

remove stakingAdmin

c653bc4

added tests and updated to one table field

b14fcb7

zhangchiqing reviewed Aug 12, 2020

View reviewed changes

PR comments

2acfb57

jordanschalm reviewed Aug 12, 2020

View reviewed changes

zhangchiqing reviewed Aug 12, 2020

View reviewed changes

adding epoch counter arguments and tests

e2a96cb

joshuahannan requested review from zhangchiqing and jordanschalm August 13, 2020 13:10

joshuahannan changed the title ~~DRAFT: Add identity table smart contract~~ Add identity table smart contract Aug 13, 2020

turbolent reviewed Aug 13, 2020

View reviewed changes

fixing init function

e5de6ea

zhangchiqing reviewed Aug 14, 2020

View reviewed changes

zhangchiqing approved these changes Aug 14, 2020

View reviewed changes

PR fixes

f613eff

zhangchiqing reviewed Aug 14, 2020

View reviewed changes

joshuahannan merged commit 09c4cc2 into master Aug 14, 2020

joshuahannan deleted the josh/identities branch September 16, 2020 17:32

-        pub fun startNewEpoch() {
+        pub fun startNewEpoch(Uint64 newEpochID) {
+          pre {
+            newEpochID == FlowIdentityTable.currentEpochID + UInt64(1): "invalid new epoch ID"
+          }
+        }

Add identity table smart contract #2

Add identity table smart contract #2

Conversation

joshuahannan commented Aug 3, 2020 • edited

turbolent left a comment

Choose a reason for hiding this comment

zhangchiqing left a comment

Choose a reason for hiding this comment

jordanschalm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuahannan commented Aug 5, 2020

jordanschalm commented Aug 5, 2020

joshuahannan commented Aug 10, 2020

jordanschalm left a comment

Choose a reason for hiding this comment

joshuahannan commented Aug 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing Aug 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing Aug 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

joshuahannan commented Aug 13, 2020

turbolent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbolent Aug 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuahannan commented Aug 3, 2020 •

edited

joshuahannan commented Aug 11, 2020 •

edited

zhangchiqing Aug 12, 2020 •

edited

zhangchiqing Aug 12, 2020 •

edited

turbolent Aug 13, 2020 •

edited