feature: scripts for fetching plan execution statuses #5

oddaf · 2024-07-24T20:41:57Z

Resolves #3

amusingaxl

Looking pretty good overall, I thought it would be way more complex than this due to LibNote.
I pointed a couple of issues with the data fetching strategy, and also we might need to figure out a way to reference the spell address itself, other than usr. A conforming spell uses the associated action as usr, however users (devs included), often refer to the spell main address.

scripts/list-pause-plans.js

amusingaxl · 2024-07-30T22:50:48Z

scripts/list-pause-plans.js

+        let tableData = prepareTableData(events, pause);
+
+        if (!argv.all && !argv.pending) {
+            tableData = tableData.length > 21 ? [tableData[0], ...tableData.slice(-20)] : tableData;


If the suggestion about data sorting by newest values first is applied, this can be simplified as:

Suggested change

tableData = tableData.length > 21 ? [tableData[0], ...tableData.slice(-20)] : tableData;

tableData = tableData.length > 21 ? tableData.slice(0, 21) : tableData;

This logic was removed in favor of the fromBlock parameter: f6e02c3

amusingaxl · 2024-07-30T22:51:50Z

scripts/list-pause-plans.js

+        if (!argv.all && !argv.pending) {
+            tableData = tableData.length > 21 ? [tableData[0], ...tableData.slice(-20)] : tableData;
+        } else if (argv.pending) {
+            tableData = tableData.filter(row => row[6] === "pending" || row[0] === "HASH");


The comparison will fail because of mismatching cases:

Suggested change

tableData = tableData.filter(row => row[6] === "pending" || row[0] === "HASH");

tableData = tableData.filter(row => row[6] === "PENDING" || row[0] === "HASH");

Also in general, I would prefer to keep the presentation layer as simple as possible and do all filtering when data is being fetched.
This allows that part of the script to be reused by a front-end, which would not need to re-implement this sort of logic.

Fixed in e37ee56, also separated all the filtering from presentation layer 56872ae and 4f032ba

amusingaxl · 2024-07-30T23:01:19Z

scripts/README.md

+
+### 2. Run scripts
+
+List the 20 most recent plans in `MCD_PAUSE`


I believe that instead of fixing an arbitrary number of events to return, we could maybe let the user specify how many blocks they want to go in the past.

However, that introduces one edge case: events for the same [usr, tag, fax, eta] might be missing because of the from block. It's usually fine, as we can consider any subsequent exec or drop to have been preceded by a plan.

Done in f6e02c3

scripts/list-pause-plans.js

scripts/README.md

amusingaxl · 2024-07-30T23:11:06Z

scripts/README.md

As commented in the README, usr usually refers to the DssSpellAction for conforming spells, not to the spell itself.
I believe the script should somehow return the associated spell address. I'm not entirely sure how to get that information, though.

[...] I'm not entirely sure how to get that information, though.

Oh, I just noticed that msg.sender is logged as topic 1:
https://vscode.blockscan.com/ethereum/0xbe286431454714f511008713973d3b053a2d38f3#L43

For conforming spells, this can be used to obtain the spell address.
Since it's not guaranteed, maybe the column should be named SENDER and the documentation should explain that this COULD be the spell address 🤷🏻‍♂️

Done in 5843d92

amusingaxl · 2024-07-31T17:17:35Z

Oh, I just noticed that there are both a script/ and a scripts/ directories now, maybe you want to move the entire content of this script into script/list-pause-plans/.

Co-authored-by: amusingaxl <[email protected]>

…to feat/scripts

… block

oddaf · 2024-08-16T01:35:01Z

Oh, I just noticed that there are both a script/ and a scripts/ directories now, maybe you want to move the entire content of this script into script/list-pause-plans/.

I renamed the folder cli to avoid confusion. The script folder contains solidity scripts (foundry) so it didn't feel right to move this there, let me know what you think.

amusingaxl · 2024-08-19T14:04:59Z

cli/list-pause-plans.js

+    if (!process.env.ETH_RPC_URL) {
+        console.warn("ETH_RPC_URL not set, falling back to a public RPC provider. For improved results set ETH_RPC_URL to a trusted node.");
+    }


Should this also be a CLI parameter? Like --eth-rpc-url?
It could default to the env var if it's set.

done in a683b60

DaiFoundation-DevOps · 2024-09-05T00:49:13Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

amusingaxl

Made some additional suggestions

amusingaxl · 2024-09-20T13:57:27Z

cli/README.md

+
+Pending plans in `MCD_PAUSE` since block 19420069
+```
+node list-pause-plans -pending --from-block 19420069


Suggested change

node list-pause-plans -pending --from-block 19420069

node list-pause-plans --pending --from-block 19420069

amusingaxl · 2024-09-20T13:58:20Z

cli/list-pause-plans.js

+        alias: 'e',
+        type: 'string',
+        description: 'Rpc URL - Falls back to ETH_RPC_URL env var, then to a public provider',
+        default: 0


If we're being faithful to the description above, we should handle that here:

Suggested change

default: 0

default: process.env.ETH_RPC_URL || 'mainnet'

amusingaxl · 2024-09-20T14:02:04Z

cli/list-pause-plans.js

+    .option('eth-rpc-url', {
+        alias: 'e',
+        type: 'string',
+        description: 'Rpc URL - Falls back to ETH_RPC_URL env var, then to a public provider',


Suggested change

description: 'Rpc URL - Falls back to ETH_RPC_URL env var, then to a public provider',

description: 'RPC URL - Falls back to `ETH_RPC_URL` env var, then to a public provider',

amusingaxl · 2024-09-20T14:15:18Z

cli/list-pause-plans.js

+    .option('eth-rpc-url', {
+        alias: 'e',


Maybe it would be better to make the options align with Foundry:

I agree can be f'ing confusing, but ¯\_(ツ)_/¯:

Suggested change

.option('eth-rpc-url', {

alias: 'e',

.option('rpc-url', {

alias: ['fork-url', 'f'],

amusingaxl · 2024-09-20T14:18:31Z

cli/list-pause-plans.js

+function getProvider() {
+    const url = argv.ethRpcUrl || process.env.ETH_RPC_URL || "mainnet";
+    if (url == "mainnet") {
+        console.warn("Falling back to a public provider. For best experience set a custom RPC URL.");
+    }
+    return ethers.getDefaultProvider(url);
+}


If the suggestion above is applied, then this whole function becomes superfluous:

Suggested change

function getProvider() {

const url = argv.ethRpcUrl || process.env.ETH_RPC_URL || "mainnet";

if (url == "mainnet") {

console.warn("Falling back to a public provider. For best experience set a custom RPC URL.");

}

return ethers.getDefaultProvider(url);

}

if (argv.rpcUrl == "mainnet") {

console.warn("Falling back to a public provider. For best experience set a custom RPC URL.");

}

amusingaxl · 2024-09-20T14:19:24Z

cli/list-pause-plans.js

+        const provider = getProvider();
+        const pause = new ethers.Contract(MCD_PAUSE, pauseABI, provider);


Suggested change

const provider = getProvider();

const pause = new ethers.Contract(MCD_PAUSE, pauseABI, provider);

const provider = getProvider();

const pause = new ethers.Contract(MCD_PAUSE, pauseABI, ethers.getDefaultProvider(argv.rpcUrl));

amusingaxl · 2024-09-20T14:22:00Z

cli/list-pause-plans.js

+    .alias('help', 'h')
+    .argv;
+
+const MCD_PAUSE = "0xbE286431454714F511008713973d3B053A2d38f3";


This most likely belongs in the pause-plan-data.js file.

amusingaxl · 2024-09-20T14:24:32Z

cli/pause-plan-data.js

+    };
+}
+
+export function prepareData(events, contract, filter) {


This function conflates data processing with data viewing.
prepareData should return an array of objects instead and let the table-formatting to the upper level.

You can and should enrich the data here, (i.e.: figuring out the status), as this part of the data logic, however it should return the data in the most agnostic way possible.

amusingaxl · 2024-09-20T14:42:32Z

cli/pause-plan-data.js

There should probably be a façade function that hides all implementation details and returns plain data in this file.

From an information ownership perspective, it should also take care of instantiating the Pause contract.

All configurable options should be passed in as an options object:

const MCD_PAUSE = "0xbE286431454714F511008713973d3B053A2d38f3"; export const PlanStatus = { ALL: 'ALL', PENDING: 'PENDING', DROPPED: 'DROPPED', EXECUTED: 'EXECUTED', } // Considering 1 block every 12.5 secs const FROM_1_YEAR_AGO = -2522880; export async function getPausePlans( provider, { fromBlock = FROM_1_YEAR_AGO, status = PlanStatus.ALL } = {}, ) { const pause = new ethers.Contract(MCD_PAUSE, pauseABI, provider); const events = await getFilteredEvents(pause, fromBlock); const filter = status === PlanStatus.ALL ? undefined : status; return prepareData(events, pause, filter); }

This way it can be more easily consumed by different clients, being it the CLI in the other script or a 3rd party UI.

amusingaxl · 2024-09-20T14:45:13Z

cli/list-pause-plans.js

+        const pause = new ethers.Contract(MCD_PAUSE, pauseABI, provider);
+
+        const events = await getFilteredEvents(pause, argv.fromBlock);
+        let tableData = prepareData(events, pause, argv.pending ? "PENDING" : null);


Instead of importing prepareData, there should be a function in this file that will turn the enriched raw data into a table structure.

amusingaxl · 2024-09-23T14:39:28Z

Oh, please remove the top-level package-lock.json file. There should be only 1 of those, within the cli/ directory.

feature: scripts for fetching plan execution statuses

31b5de0

oddaf requested review from amusingaxl and 0xdecr1pto July 24, 2024 20:42

amusingaxl requested changes Jul 30, 2024

View reviewed changes

oddaf and others added 13 commits August 14, 2024 21:23

refactor: Move event filtering to the query

ce24f31

refactor: Sort list by ETA, desplay most recent on top

daf06e7

Update scripts/README.md

23e4b28

Co-authored-by: amusingaxl <[email protected]>

Update scripts/README.md

7a8774b

Co-authored-by: amusingaxl <[email protected]>

fix: Pending filter

e37ee56

refactor: table width

5db40df

fix: pending filter index

2ea1010

refactor: dissociate presentation

4f032ba

Merge branch 'feat/scripts' of https://github.com/makerdao/protego in…

41ea82d

…to feat/scripts

refactor: include spell address in the report

5843d92

feat: Accept a block number param, display only spells from the given…

f6e02c3

… block

refactor: separate display logic from data fetching/filtering logic

56872ae

refactor: rename cli scripts folder

c409944

amusingaxl reviewed Aug 19, 2024

View reviewed changes

oddaf added 3 commits September 4, 2024 20:59

refactor: move data treating functions to separate file

e8f7944

fix: delete old files

6dd5ce3

feat: allow for inline eth-rpc-url setting

a683b60

amusingaxl reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: scripts for fetching plan execution statuses #5

feature: scripts for fetching plan execution statuses #5

oddaf commented Jul 24, 2024

amusingaxl left a comment

amusingaxl Jul 30, 2024

oddaf Aug 16, 2024

amusingaxl Jul 30, 2024

amusingaxl Jul 30, 2024

oddaf Aug 16, 2024

amusingaxl Jul 30, 2024

oddaf Aug 16, 2024

amusingaxl Jul 30, 2024

amusingaxl Jul 30, 2024

oddaf Aug 16, 2024

amusingaxl commented Jul 31, 2024

oddaf commented Aug 16, 2024

amusingaxl Aug 19, 2024

oddaf Sep 5, 2024

DaiFoundation-DevOps commented Sep 5, 2024

amusingaxl left a comment

amusingaxl Sep 20, 2024

amusingaxl Sep 20, 2024

amusingaxl Sep 20, 2024

amusingaxl Sep 20, 2024

amusingaxl Sep 20, 2024

amusingaxl Sep 20, 2024

amusingaxl Sep 20, 2024

amusingaxl Sep 20, 2024

amusingaxl Sep 20, 2024 •

edited

Loading

amusingaxl Sep 20, 2024

amusingaxl commented Sep 23, 2024

	tableData = tableData.length > 21 ? [tableData[0], ...tableData.slice(-20)] : tableData;
	tableData = tableData.length > 21 ? tableData.slice(0, 21) : tableData;

	tableData = tableData.filter(row => row[6] === "pending" \|\| row[0] === "HASH");
	tableData = tableData.filter(row => row[6] === "PENDING" \|\| row[0] === "HASH");


		### 2. Run scripts

		List the 20 most recent plans in `MCD_PAUSE`

	node list-pause-plans -pending --from-block 19420069
	node list-pause-plans --pending --from-block 19420069

	description: 'Rpc URL - Falls back to ETH_RPC_URL env var, then to a public provider',
	description: 'RPC URL - Falls back to `ETH_RPC_URL` env var, then to a public provider',

		const provider = getProvider();
		const pause = new ethers.Contract(MCD_PAUSE, pauseABI, provider);

feature: scripts for fetching plan execution statuses #5

Are you sure you want to change the base?

feature: scripts for fetching plan execution statuses #5

Conversation

oddaf commented Jul 24, 2024

amusingaxl 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

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

amusingaxl commented Jul 31, 2024

oddaf commented Aug 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaiFoundation-DevOps commented Sep 5, 2024

amusingaxl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amusingaxl Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amusingaxl commented Sep 23, 2024

amusingaxl Sep 20, 2024 •

edited

Loading