Implement Grain Integration functionality in the CLI #3085

topocount · 2021-06-01T22:05:26Z

Details

This officially hooks up grain integrations into something accessible via the CLI!

I'm open to setting up a CLI flag in addition to allowing the invocation of
grain integrations at the config level as well.

These changes also contain a semantic change for the grain integration
typing. The function exported by a grain integration is now named
GrainIntegrationFunction
The original GrainIntegration type which used to represent the
function is now as follows:

type GrainIntegration = {|
  function: GrainIntegrationFunction,
  name: string
|}

Merge Plan

Merge after #3079

Test Plan

yarn build
from repo root: cd packages/sourcecred/sharness/__snapshots__/test-instance
scdev grain -f
open the new file in the output/grainIntegration and observe 2 lines
of csv output. The first column containing payout addresses, the
second column containing payout amounts in BigInt form.

Next Steps

All we need to use it is a way to easily set payout addresses for
participants. This was done by hand in a NodeJS repl in order to
enable the above test case.

blueridger · 2021-06-02T18:40:43Z

packages/sourcecred/src/cli/grain.js

+  const integrationCurrency = grainInput.currencyDetails.integrationCurrency;
+  const grainIntegration = grainInput.grainConfig.integration;
+  if (integrationCurrency && grainIntegration) {
+    distributions.forEach((distribution) => {
+      const result = executeGrainIntegration(
+        ledger,
+        grainIntegration.function,
+        distribution,
+        integrationCurrency,
+        false
+      );
+      if (result.grainIntegrationOutput)
+        instance.writeGrainIntegrationOutput(
+          result.grainIntegrationOutput,
+          distribution
+        );
+    });
+  }


Shouldn't this go in the grain API instead of the grain CLI since it is business logic?

good point. I'll separate the business logic from the I/O

packages/sourcecred/src/api/instance/localInstance.js

blueridger · 2021-08-18T18:55:44Z

packages/sourcecred/src/api/main/grain.js

+  if (integrationCurrency && grainIntegration) {
+    distributions.forEach((distribution) => {
+      const result = executeGrainIntegration(
+        ledger,
+        grainIntegration.function,
+        distribution,
+        integrationCurrency,
+        false
+      );
+      results.push(result);
+    });
+  }
+  return results;
+}


Seems strange for this to return an array of objects that each contain the same ledger, and that in the CLI we encode the assumption that the ledger we passed in is going to be mutated and so we can ignore the ledgers that come out. What about a refactor that looks more like:

Suggested change

if (integrationCurrency && grainIntegration) {

distributions.forEach((distribution) => {

const result = executeGrainIntegration(

ledger,

grainIntegration.function,

distribution,

integrationCurrency,

false

);

results.push(result);

});

}

return results;

}

if (integrationCurrency && grainIntegration) {

let ledgerResult = ledger;

distributions.forEach((distribution) => {

const {ledger: newLedger, output, distributionCredTimestamp} = executeGrainIntegration(

ledger,

grainIntegration.function,

distribution,

integrationCurrency,

false

);

results.push({output, distributionCredTimestamp});

ledgerResult = newLedger;

});

}

return {ledger: ledgerResult, results};

}

yeah good catch. We don't ever want to assume it's safe to mutate the same ledger reference across multiple function calls.

blueridger · 2021-08-18T18:57:04Z

packages/sourcecred/src/cli/grain.js

  const {distributions, ledger} = await grain(grainInput);

+  const grainIntegrationResults = executeGrainIntegrationsFromGrainInput(
+    grainInput,
+    ledger,
+    distributions
+  );


This is related to my other comment.

Suggested change

const {distributions, ledger} = await grain(grainInput);

const grainIntegrationResults = executeGrainIntegrationsFromGrainInput(

grainInput,

ledger,

distributions

);

const {distributions, ledger: ledgerBeforeIntegrations} = await grain(grainInput);

const {ledger, results: grainIntegrationResults} = executeGrainIntegrationsFromGrainInput(

grainInput,

ledgerBeforeIntegrations,

distributions

);

This officially hooks up grain integrations into something accessible! All we need to use it is a way to easily set payout addresses for participants. These changes also contain a semantic change for the grain integration typing. The function exported by a grain integration is now named `GrainIntegrationFunction` The original `GrainIntegration` type which used to represent the function is now as follows: ```js type GrainIntegration = {| function: GrainIntegrationFunction, name: string |} ``` This will allow core to utilize the configured grain integration name internally, if desired. Merge Plan: Merge after #3079 Test Plan: 0. `scdev build` 1. from repo root: `cd packages/sourcecred/sharness/__snapshots__/test-instance` 2. scdev grain -f 3. open the new file in the output/grainIntegration and observe 2 lines of csv output. The first column containing payout addresses, the second column containing payout amounts in bigint form.

Business Logic should be agnostic of I/O so it can be utilized elsewhere. This also lead to a minor API cleanup: Each distribution's credtimestamp is now included in the integration result, so integration results now have all the identifying details needed to make them unique.

blueridger · 2021-08-24T22:41:54Z

packages/sourcecred/src/api/main/grain.js

+      results.push(result);
+    });
+  }
+  return {ledger: ledgerResult, results};


Still seems weird / too heavy that we are returning a bunch of ledgers in results right here. Is there a reason for it, or can we slim down and toss the extra references to garbage collection?

blueridger · 2021-08-31T21:53:41Z

packages/sourcecred/src/api/main/grain.js

@@ -24,8 +25,15 @@ export type GrainOutput = {|
  +ledger: Ledger,
 |};

+// Similar to GrainIntegrationResult but excludes the ledger
+// since only the most recent ledger is relevant
+export type GrainIntegrationMultiResult = {|


nit: Why multi? Makes me think it's going to be an array. Not sure I have a better suggestion, I'm all turned around on the namings in this epic lol. timeStampedGrainIntegrationOutput? nah the output is optional... idk. you can keep it.

blueridger · 2021-08-31T21:58:35Z

packages/sourcecred/src/api/instance/instance.js

@@ -47,5 +47,7 @@ export interface Instance extends ReadOnlyInstance {
  writeLedger(ledger: Ledger): Promise<void>;

  /** writes grain integration output */
-  writeGrainIntegrationOutput(result: GrainIntegrationResult): Promise<void>;
+  writeGrainIntegrationOutput(
+    result: $Shape<GrainIntegrationMultiResult>


why shape? seems unnecessary since the GrainIntegrationOutput attribute is optional already

I wanted to be unopinionated enough to accept the GrainIntegrationResulttype.

topocount force-pushed the 4-grainIntegration-IO branch from 44cce43 to a7046cf Compare June 1, 2021 22:14

topocount force-pushed the 3-integration-configs branch from e766631 to 8fbffa7 Compare June 2, 2021 16:48

topocount force-pushed the 4-grainIntegration-IO branch 2 times, most recently from 3714583 to 4ca7e28 Compare June 2, 2021 17:23

topocount requested a review from blueridger June 2, 2021 17:27

blueridger reviewed Jun 2, 2021

View reviewed changes

topocount force-pushed the 3-integration-configs branch 2 times, most recently from 39b467e to 2f68004 Compare August 16, 2021 20:30

topocount force-pushed the 4-grainIntegration-IO branch from 4ca7e28 to 3fa71ac Compare August 17, 2021 18:05

topocount requested a review from blueridger August 17, 2021 20:32

blueridger reviewed Aug 18, 2021

View reviewed changes

Base automatically changed from 3-integration-configs to main August 18, 2021 21:32

topocount added 3 commits August 24, 2021 16:17

Utilize the toIso utility function to stringify date

2259338

topocount force-pushed the 4-grainIntegration-IO branch from d480be8 to db1cd4d Compare August 24, 2021 21:17

Operate on the latest ledger returned

dab95f0

topocount force-pushed the 4-grainIntegration-IO branch from db1cd4d to dab95f0 Compare August 24, 2021 21:18

topocount requested a review from blueridger August 24, 2021 21:24

blueridger reviewed Aug 24, 2021

View reviewed changes

Create GrainIntegrationMultiResult that omits a ledger

27c91ef

blueridger approved these changes Aug 31, 2021

View reviewed changes

topocount merged commit 435f6c3 into main Sep 8, 2021

topocount deleted the 4-grainIntegration-IO branch September 8, 2021 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Grain Integration functionality in the CLI #3085

Implement Grain Integration functionality in the CLI #3085

topocount commented Jun 1, 2021 •

edited

blueridger Jun 2, 2021

topocount Aug 17, 2021

blueridger Aug 18, 2021

topocount Aug 24, 2021 •

edited

blueridger Aug 18, 2021

blueridger Aug 24, 2021

blueridger Aug 31, 2021 •

edited

blueridger Aug 31, 2021

topocount Sep 8, 2021

Implement Grain Integration functionality in the CLI #3085

Implement Grain Integration functionality in the CLI #3085

Conversation

topocount commented Jun 1, 2021 • edited

Details

Merge Plan

Test Plan

Next Steps

blueridger Jun 2, 2021

Choose a reason for hiding this comment

topocount Aug 17, 2021

Choose a reason for hiding this comment

blueridger Aug 18, 2021

Choose a reason for hiding this comment

topocount Aug 24, 2021 • edited

Choose a reason for hiding this comment

blueridger Aug 18, 2021

Choose a reason for hiding this comment

blueridger Aug 24, 2021

Choose a reason for hiding this comment

blueridger Aug 31, 2021 • edited

Choose a reason for hiding this comment

blueridger Aug 31, 2021

Choose a reason for hiding this comment

topocount Sep 8, 2021

Choose a reason for hiding this comment

topocount commented Jun 1, 2021 •

edited

topocount Aug 24, 2021 •

edited

blueridger Aug 31, 2021 •

edited