New Contract: Roles #131

donchate · 2022-01-03T04:07:59Z

Description

This contract provides a stake-based remuneration capability for a DAO to establish community roles.
Community members may then, in a decentralized fashion, vie for positions based on stake-weighted approval gained from the voting token stakers.

Candidates are ranked by approval weight in the configured voting token. A number of regular (main) ranked slots receive recurring remuneration, and a number of alternate (backup) ranked slots receive remuneration using a random rotation of remaining eligible candidates. Token balances are fully distributed on each tick, so funds must be provided using manual deposit, or other smart contract (i.e DTF, Distribution).

PR Details

Does hook into tokens contract for stake weighted state updates
Unit testing intended to cover integral actions / functions within the contract. Integration testing (DTF, Distribution funding) TBD in testnet. Hope to do this concurrently with PR review.

bt-cryptomancer · 2022-01-06T02:05:45Z

contracts/roles.js

+actions.createSSC = async () => {
+  const tableExists = await api.db.tableExists('instances');
+  if (tableExists === false) {
+    await api.db.createTable('instances', ['id', 'lastTickTime']);


I think you can remove 'id' from ['id', 'lastTickTime']

If you were intending to add an index for the implicit MongoDB _id field (I'm assuming id without the underscore is a typo), there's always an index for that and you don't have to add it explicitly.

Yes, I think we should be careful here, I've seen parts using .id vs ._id. It does seem like we want to rely on the auto index _id for the instance table.

eonwarped · 2022-01-06T06:06:11Z

contracts/roles.js

+    }
+
+    const voteTokenObj = await api.db.findOneInTable('tokens', 'tokens', { symbol: voteToken });
+    if (!api.assert(voteTokenObj && voteTokenObj.stakingEnabled, 'voteToken must have staking enabled')) return;


We should establish a limit for the # of instances to prevent mass staking events from taking too much time. We should probably also limit this to vote token issuers because of that.

Likely we can make the check about the number of active instances for a given token , and make sure we only do staking updates for active instances.

I don't think we should ever allow there to be a potentially unbounded number of operations happening when there's a staking update, there should always be some well-defined upper limit that we are comfortable with. Otherwise there should be a mechanism to split update processing across multiple blocks, with intermediate state being stored between blocks.

eonwarped · 2022-01-06T14:36:47Z

contracts/roles.js

+  if (processQueryLimit) {
+    if (!api.assert(typeof processQueryLimit === 'string' && api.BigNumber(processQueryLimit).isInteger() && api.BigNumber(processQueryLimit).gte(1), 'invalid processQueryLimit')) return;
+    params.processQueryLimit = api.BigNumber(processQueryLimit).toNumber();
+  }


All of these can be integer type, and can be checked with Number.isInteger() (see other contracts)

eonwarped · 2022-01-06T14:40:23Z

contracts/roles.js

+
+  if (api.assert(authorizedUpdate, 'you must have enough tokens to cover the update fee')
+    && api.assert(isSignedWithActiveKey === true, 'you must use a transaction signed with your active key')
+    && api.assert(typeof instanceId === 'string' && api.BigNumber(instanceId).isInteger(), 'invalid instanceId')


same here, can use integer check

No need to cast things toNumber everywhere else then

eonwarped · 2022-01-06T14:42:57Z

contracts/roles.js

+    for (let i = 0; i < roles.length; i += 1) {
+      const role = roles[i];
+      if (!api.assert(Object.keys(role).length === 5
+        && typeof role.name === 'string' && role.name.length < 50


split these into their own error messages?

eonwarped · 2022-01-06T14:48:14Z

contracts/roles.js

+        await api.executeSmartContract('tokens', 'transfer', {
+          to: 'null', symbol: inst.candidateFee.symbol, quantity: inst.candidateFee.amount,
+        });
+      } else if (inst.candidateFee.method === 'issuer') {


Perhaps we can consolidate these by having them specify a 'feeAccount', and that will cover both of these cases and allow a different recipient as well

I like this idea, more flexibility is always good and it feels slightly less clunky from a usage perspective. Also, as per my other comments fees should be handled & verified before doing the insert.

eonwarped · 2022-01-06T15:06:30Z

contracts/roles.js

+    const roleTickTime = api.BigNumber(blockDate.getTime())
+      .minus(role.tickHours * 3600 * 1000).toNumber();
+
+    if (role.lastTickTime <= roleTickTime) {


this could be part of the query as well? currently it checks <= instTickTime but we can have it be <= min(two tick times)

eonwarped · 2022-01-06T15:13:07Z

contracts/roles.js

+              .toFixed(voteTokenObj.precision, api.BigNumber.ROUND_HALF_UP);
+
+            if (candidate.active === true) {
+              if (funded.length < role.mainSlots || api.BigNumber(backupWeight).lte(accWeight)) {


The backup mechanism seems a bit weird, or works only when numBackup = 1. If there are more than 1 backups requested, and it generates a single number, it'll just pick two backups together without re-rolling the die? I think this might be similar to what witness contract does (Which is also wrong)

Makes sense since it was copied from witness contract. Going to rework.

eonwarped · 2022-01-06T15:15:46Z

test/roles.js

+
+      await fixture.sendBlock(block);
+
+      // let res = await fixture.database.getLatestBlockInfo();


eonwarped · 2022-01-06T15:17:03Z

test/roles.js

+        done();
+      });
+  });  
+


does this test deposits via dtf/distribution contracts?

Not currently, I was going to confirm in testnet

eonwarped · 2022-01-06T15:17:20Z

test/roles.js

+  await tableAsserts.assertNoErrorInLastBlock();
+}
+
+// distribution test suite


roles test suite

bt-cryptomancer · 2022-01-07T03:08:47Z

contracts/roles.js

+    };
+    await api.db.update('candidates', candidate);
+
+    const role = await api.db.findOne('roles', { _id: candidate.roleId });


if deltaToken is defined, then this is a redundant query (you've already looked up the role for deltaToken processing above). It would be more efficient to re-use the earlier query results in that case, and only execute the query again here if deltaToken is not defined.

bt-cryptomancer · 2022-01-07T03:22:05Z

contracts/roles.js

+    if (api.sender !== api.owner
+        && api.sender !== 'null'
+        && api.BigNumber(instanceCreationFee).gt(0)) {
+      await api.executeSmartContract('tokens', 'transfer', {


Although the possibility of this transfer failing is remote, it would be safer to transfer the fees before creating the new instance, and verify that the transfer succeeded. See, for example, the technique used in the issue action of the nft contract.

bt-cryptomancer · 2022-01-07T03:23:52Z

contracts/roles.js

+    if (api.sender !== api.owner
+        && api.sender !== 'null'
+        && api.BigNumber(instanceUpdateFee).gt(0)) {
+      await api.executeSmartContract('tokens', 'transfer', {


Same as above, should handle the fees and verify success before updating the instance.

bt-cryptomancer · 2022-01-07T03:26:06Z

contracts/roles.js

+    if (api.sender !== api.owner
+        && api.sender !== 'null'
+        && api.BigNumber(roleCreationFee).gt(0)) {
+      await api.executeSmartContract('tokens', 'transfer', {


Same as above, should handle the fees and verify success first.

bt-cryptomancer · 2022-01-07T03:27:46Z

contracts/roles.js

+    if (api.sender !== api.owner
+        && api.sender !== 'null'
+        && api.BigNumber(roleUpdateFee).gt(0)) {
+      await api.executeSmartContract('tokens', 'transfer', {


Same as above, should handle the fees and verify success first.

bt-cryptomancer · 2022-01-07T03:35:00Z

contracts/roles.js

+// deposit
+
+async function updateTokenBalances(role, token, quantity) {
+  const upRole = role;


Why use this upRole variable here instead of just directly using the passed in role ?

bt-cryptomancer · 2022-01-07T03:39:04Z

contracts/roles.js

+  }
+};
+
+actions.receiveDistTokens = async (payload) => {


The code for this function is almost exactly the same as for receiveDtfTokens. Perhaps you could introduce a helper function used by both of these, to avoid so much code duplication?

bt-cryptomancer · 2022-01-07T03:54:57Z

test/roles.js

+      await assertWeightConsistency(3, 'PRO');
+      await assertWeightConsistency(4, 'PRO');
+
+      assert.ok(res.virtualTransactions.length > 0, 'Expected to find virtualTransactions');


For this test case, how are the ticks set up to generate these virtual transactions? Maybe I missed it, but I didn't see any code to configure the per-block ticking behavior.

bt-cryptomancer

Overall logical structure & organization looks fine, but the devil is in the details as they say. Please see my comments at specific code lines (and respond to all of eonwarped's comments).

Also, I'd like you to add some clear release instructions to the PR in regards to how to configure / setup the per-block ticking behavior for virtual transactions. I think I remember we moved all that to config so no code changes would be needed to the main node software, but I don't recall all the details and don't want to be trying to figure it out when I'm in the middle of releasing this.

donchate added 6 commits January 3, 2022 04:00

roles contract initial CRUD

89dcada

stake weight and tick implementation and test

3635d60

cleaned up for testnet

218c9c3

throttling implementation

9985635

throttling test, general cleanup

4c3e5e5

pre testnet cleanup

29d76eb

bt-cryptomancer reviewed Jan 6, 2022

View reviewed changes

eonwarped requested changes Jan 6, 2022

View reviewed changes

bt-cryptomancer reviewed Jan 7, 2022

View reviewed changes

bt-cryptomancer requested changes Jan 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Contract: Roles #131

New Contract: Roles #131

donchate commented Jan 3, 2022

bt-cryptomancer Jan 6, 2022

eonwarped Jan 6, 2022

eonwarped Jan 6, 2022

eonwarped Jan 6, 2022

bt-cryptomancer Jan 7, 2022

eonwarped Jan 6, 2022

eonwarped Jan 6, 2022

eonwarped Jan 6, 2022

eonwarped Jan 6, 2022

eonwarped Jan 6, 2022

bt-cryptomancer Jan 7, 2022

eonwarped Jan 6, 2022

eonwarped Jan 6, 2022

donchate Jan 7, 2022

eonwarped Jan 6, 2022

eonwarped Jan 6, 2022

donchate Jan 7, 2022

eonwarped Jan 6, 2022

bt-cryptomancer Jan 7, 2022

bt-cryptomancer Jan 7, 2022 •

edited

Loading

bt-cryptomancer Jan 7, 2022

bt-cryptomancer Jan 7, 2022

bt-cryptomancer Jan 7, 2022

bt-cryptomancer Jan 7, 2022

bt-cryptomancer Jan 7, 2022

bt-cryptomancer Jan 7, 2022

bt-cryptomancer left a comment


		await fixture.sendBlock(block);

		// let res = await fixture.database.getLatestBlockInfo();

New Contract: Roles #131

Are you sure you want to change the base?

New Contract: Roles #131

Conversation

donchate commented Jan 3, 2022

Description

PR Details

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

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

bt-cryptomancer Jan 7, 2022 • edited Loading

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

bt-cryptomancer left a comment

Choose a reason for hiding this comment

bt-cryptomancer Jan 7, 2022 •

edited

Loading