Feature: Disable balance accounting in the ledger #3192

topocount · 2021-09-21T15:41:31Z

Description

When utilizing an on chain grain integration, tracking account balances
in the SourceCred ledger makes little sense. These changes disable
balance accounting, while preserving the functionality key to
SourceCred, which is tracking distributions across identities. While
balances are no longer tracked, paid totals will continue to be. This
feature, combined with Distribution execution tracking, narrows the
scope of the ledgers focus to the state of distributions, and whether or
not they have been executed outside of SourceCred.

When account balance tracking is disabled, the following changes are
made to ledger behavior:

all grain balances are zeroed out in mutable accounts tracked by the
ledger
grain transfers are disabled
The ledger stores the currently configured integrationCurrency at the instant
accounting is disabled as externalCurrency. This allows for the configuration
to be historically tracked in case the configuration is accidentally changed, and
allows for more predictable behavior in the ledger in this mode. If the integrationCurrency
needs to be updated, the disableAccounting method can just be re-run with the
new currency to update the configuration within the ledger.
distributed grain is not added to a user's balance. the paid total is
updated, however. Undistributed balances are tracked as a whole through
the distribution tracking functionality added earlier.
identities without a relevant payout address will be deactivated
identities that have no relevant payout addresses configured cannot be
activated. relevant means the external currency configured in the
ledger's state.

Now that all integration functionality has settled in the ledger, the executeGrainIntegration
interface can be simplified and cleaned up. No information has been
lost; Config details can now simply be queried from the ledger directly.

Config inconsistencies are now tracked in the main/grain.js API file.
The main api will be tracking changes to the external currency config and
update the ledger dynamically before any integrations run. This seemed like
the best compromise between "magic" and usability. I think it's important that
the ledger tracks all integration currency changes for the purpose of transparency,
but also using a config file seems to be the best input method for now. This
tracking can be deprecated when we migrate to a webUI for config management.

test plan: unit tests have been updated

Test Plan

Unit tests included

Merge Plan

merge plan can be found here

Future Work

The current entry point is integration currency config in the grain.json config file. However, I'd really like to set up a CLI dialog for setting this and other integration config in the near-term. This is a hard requirement for the smart contract deployment for the merkle distribution integration, so I might as well include these few extra dialogs as a part of that story.

Update: I ended up just creating config entries in config/grain.json that are checked against the ledger every time grain distribution runs via the CLI.

blueridger

I'm a little confused about what the entry point for this is going to be. Can you add some insight to the description? Like is there going to be a UI button that calls setExternalCurrency? What entry points will there be for disableAccounting?

I have some pretty structural comments in this review, so I will review unit tests once those are settle. First comment you should address: #3192 (comment)

packages/sourcecred/src/core/ledger/ledger.js

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

packages/sourcecred/src/core/ledger/ledger.js

topocount · 2021-11-02T02:20:55Z

I'm a little confused about what the entry point for this is going to be. Can you add some insight to the description? Like is there going to be a UI button that calls setExternalCurrency? What entry points will there be for disableAccounting?

@blueridger thanks for requesting this. Just added the following to the description:

The current entry point is integration currency config in the grain.json config file. However, I'd really like to set up a CLI dialog for setting this and other integration config in the near-term. This is a hard requirement for the smart contract deployment for the merkle distribution integration, so I might as well include these few extra dialogs as a part of that story.

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

packages/sourcecred/src/api/grainConfig.js

blueridger

For best clarity, I recommend that you add a consistent comment structure for all 4 of the new actions (both enable/set and disable/remove) of the form:

When <the other> is enabled: effect/behavior
When <the other> is disabled: effect/behavior

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

packages/sourcecred/src/core/ledger/grainIntegration.js

packages/sourcecred/src/core/ledger/ledger.js

When utilizing an on chain grain integration, tracking account balances in the sourcecred ledger makes little sense. These changes disable balance accounting, while preserving the functionality key to SourceCred, which is tracking distributions across identities. While balances are no longer tracked, paid totals will continue to be. This feature, combined with Distribution execution tracking, narrows the scope of the ledgers focus to the state of distributions, and whether or not they have been executed outside of SourceCred. When account balance tracking is disabled, the following changes are made to ledger behavior: - all grain balances are zeroed out in mutable accounts tracked by the ledger - grain transfers are disabled - distributed grain is not added to a user's balance. the paid total is updated, however. Undistributed balances are tracked as a whole through the distribution tracking functionality added earlier. - identities without a relevant payout address will be deactivated - identities that have no relevant payout addresses configured cannot be activated. relevant means the external currency configured in the ledger's state. Test plan: Unit tests included

Now that all intergration funcitonality has settled in the ledger, this interface can be simplified and cleaned up. No information has been lost; Config details can now simply be queried from the ledger directly. Config inconsistencies are now tracked in the main/grain.js API file. The main api will be tracking changes to the external currency config and update the ledger dynamically before any integrations run. test plan: unit tests have been updated

Completely decoupled setting External Currencies from disabling accounting. However, disabling accounting still requires an external currency is set. Cleaned up a bunch of cruft that I missed from the other changes

The new design centers the ledger as an audit log. All config changes (except for the sink identity) should be recorded in the ledger before any grain processing occurs. This setup is definitely inelegant, and I'm looking forward to migrating away from it when we can manage configs via some UI instead. test plan: unit tests are updated for the config parser

- update docstrings to clearly communicate config effects - naming functions - ordering of functions

Ledger config was happening after the first grain distribution executed. Now it runs as a hook before any grain distributions run.

blueridger

It looks like you are using git please even though you are committing new changes as new commits. This makes it a little harder to track which commits are new. I'd recommend just using git push if you are not using git commit --amend

packages/sourcecred/src/cli/grain.js

packages/sourcecred/src/core/ledger/ledger.js

blueridger · 2021-11-24T20:10:30Z

packages/sourcecred/src/core/ledger/ledger.js

+      if (!this._balanceAccountingEnabled) {
+        this._deactivateAccountsWithoutPayoutAddress();
+      }


This should go in the private method under the mutations line, right?

it can't. Calls to public methods from private "processAction" methods that both ultimtely append to the ledger result in out-of-order timestamps.

This is an acceptable location because it only calls public methods under the hood. I'd like to keep this as a private method even though it only invokes a public method though because I don't want it to be a documented part of the API right now.

packages/sourcecred/src/core/ledger/ledger.js

topocount · 2021-11-26T15:07:44Z

It looks like you are using git please even though you are committing new changes as new commits. This makes it a little harder to track which commits are new. I'd recommend just using git push if you are not using git commit --amend

@blueridger this PR has been open for a while, and I rebased to keep the changes up to date with the most recent updates in main. I've been appending changes via new commits, since rebasing doesn't change the date of a commit, it should be possbile to page into the relevant changes by referencing dates. I'll post deep links containing the relevant commits going forward.

blueridger · 2021-11-26T16:08:02Z

Ahhh I see. No need to deep link, I can look for the timestamps. Thanks.

A grain integration might want to store some details within an instance, such as a smart contract address. This update allows arbitrary updates to a scoped object within the grain config file. Test Plan: Unit tests are provided at the grain integration interface. Integration testing will be demonstrated in the test instance using the merkle integration.

topocount requested a review from blueridger September 21, 2021 15:41

topocount linked an issue Oct 25, 2021 that may be closed by this pull request

Implement Ledger accountingEnabled setting #3168

Closed

topocount force-pushed the accounting branch 3 times, most recently from 9d87eb4 to d6021e6 Compare November 1, 2021 18:13

blueridger reviewed Nov 1, 2021

View reviewed changes

topocount commented Nov 2, 2021

View reviewed changes

packages/sourcecred/src/api/main/grain.js Outdated Show resolved Hide resolved

topocount force-pushed the accounting branch from 7b21082 to 5134748 Compare November 2, 2021 23:02

topocount requested a review from blueridger November 4, 2021 14:47

topocount force-pushed the accounting branch from 5134748 to 717687b Compare November 9, 2021 16:01

blueridger reviewed Nov 11, 2021

View reviewed changes

packages/sourcecred/src/api/grainConfig.js Outdated Show resolved Hide resolved

blueridger reviewed Nov 11, 2021

View reviewed changes

topocount added 7 commits November 24, 2021 07:18

Update the ledger after review from @blueridger

98511d8

Completely decoupled setting External Currencies from disabling accounting. However, disabling accounting still requires an external currency is set. Cleaned up a bunch of cruft that I missed from the other changes

Further review changes

2c4302f

- update docstrings to clearly communicate config effects - naming functions - ordering of functions

Accounting bugfix: Fix recursive call during batch ledger config

8086a71

Accounting configuration bugfix: configure Ledger called late

a96c7e9

Ledger config was happening after the first grain distribution executed. Now it runs as a hook before any grain distributions run.

topocount force-pushed the accounting branch from 19299b1 to a96c7e9 Compare November 24, 2021 13:22

topocount mentioned this pull request Nov 24, 2021

Merkle Tree gas-efficient Grain Integration #3139

Open

topocount requested a review from blueridger November 24, 2021 17:20

blueridger reviewed Nov 24, 2021

View reviewed changes

Further DocsString and readability improvements

02bb6e9

topocount force-pushed the accounting branch from 8615b20 to 02bb6e9 Compare November 26, 2021 16:30

topocount requested a review from blueridger November 26, 2021 16:39

blueridger approved these changes Dec 3, 2021

View reviewed changes

topocount merged commit b5709e6 into main Dec 13, 2021

topocount deleted the accounting branch December 13, 2021 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Disable balance accounting in the ledger #3192

Feature: Disable balance accounting in the ledger #3192

topocount commented Sep 21, 2021 •

edited

blueridger left a comment •

edited

topocount commented Nov 2, 2021 •

edited

blueridger left a comment

blueridger left a comment

blueridger Nov 24, 2021

topocount Nov 26, 2021 •

edited

blueridger Nov 26, 2021

topocount commented Nov 26, 2021

blueridger commented Nov 26, 2021

Feature: Disable balance accounting in the ledger #3192

Feature: Disable balance accounting in the ledger #3192

Conversation

topocount commented Sep 21, 2021 • edited

Description

Test Plan

Merge Plan

Future Work

blueridger left a comment • edited

Choose a reason for hiding this comment

topocount commented Nov 2, 2021 • edited

blueridger left a comment

Choose a reason for hiding this comment

blueridger left a comment

Choose a reason for hiding this comment

blueridger Nov 24, 2021

Choose a reason for hiding this comment

topocount Nov 26, 2021 • edited

Choose a reason for hiding this comment

blueridger Nov 26, 2021

Choose a reason for hiding this comment

topocount commented Nov 26, 2021

blueridger commented Nov 26, 2021

topocount commented Sep 21, 2021 •

edited

blueridger left a comment •

edited

topocount commented Nov 2, 2021 •

edited

topocount Nov 26, 2021 •

edited