automate liquidation testing #36

rabi-siddique · 2024-12-17T08:59:54Z

This PR introduces a GitHub Actions workflow to automate the testing of liquidation data indexing. The workflow:

Launches an A3P container.
Sets up the SubQL indexer.
Configures three vaults, places three bids, and changes the ATOM price to trigger the liquidation process.

This setup to initiate liquidation is similar to the one in the automated tests in dapp-inter.

After the liquidation process is completed, we make requests to the GraphQL server to retrieve data related to liquidations. We then verify that these data values match our expected values.

turadg

Congrats on getting this working.

I left some requests that I expect before merging to main but this PR is merging to rs-test so I'll approve that and I'll look for the changes in #35 before approving that for main.

turadg · 2024-12-18T14:49:09Z

.github/scripts/changePrice.mjs

+  process.exit(1);
+}
+
+const setAtomPrice = async (amount, containerName, agoricNet) => {


this function basically is the script. I think it would be best to inline it.

maybe you made it a function because of the await? mjs in Node supports top-level await

i can see a case for calling main() in the last line like you do in the other file, but I think it's best not to parameterize that since the values are coming from env

turadg · 2024-12-18T14:50:01Z

.github/scripts/changePrice.mjs

@@ -0,0 +1,35 @@
+import { execa } from 'execa';


since this is a script, please include the shebang so it can be run from shell

Suggested change

import { execa } from 'execa';

#! /usr/bin/env node

import { execa } from 'execa';

turadg · 2024-12-18T14:52:06Z

.github/scripts/changePrice.mjs

+const amount = process.env.amount;
+const containerName = process.env.containerName;
+const agoricNet = process.env.agoricNet;


it's surprising to see this different casing for AGORIC_NET but I see how it makes the workflow read better.

though please simplify the expression here with destructuring,

Suggested change

const amount = process.env.amount;

const containerName = process.env.containerName;

const agoricNet = process.env.agoricNet;

const { agoricNet, amount, containerName } = process.env;

turadg · 2024-12-18T14:52:47Z

.github/scripts/changePrice.mjs

+    const env = {
+      AGORIC_NET: agoricNet,
+    };


if you have consistent casing,

Suggested change

const env = {

AGORIC_NET: agoricNet,

};

const env = {

AGORIC_NET,

};

turadg · 2024-12-18T14:57:53Z

.github/scripts/checkLiquidation.mjs

@@ -0,0 +1,157 @@
+// @ts-check
+import fetch from 'node-fetch';
+import assert from 'assert';


I think you always want strict. just import that version so these read more cleanly.

Suggested change

import assert from 'assert';

import assert from 'node:assert/strict';

turadg · 2024-12-18T14:58:22Z

.github/scripts/checkLiquidation.mjs

@@ -0,0 +1,157 @@
+// @ts-check
+import fetch from 'node-fetch';


fetch is global in modern Node.

Suggested change

import fetch from 'node-fetch';

/* eslint-env node */

Then below you can use fetch without import.

turadg · 2024-12-18T15:02:35Z

.github/scripts/checkLiquidation.mjs

+  expectedIds,
+  expectedDebts,
+  expectedBalance,
+  expectedDenom,


these parameters aren't really independent of each other. expectedLiquidating and expectedLiquidated have the same structure. please just pass in that object.

turadg · 2024-12-18T15:07:27Z

.github/scripts/checkLiquidation.mjs

+      // Validate Ids
+      assert.strictEqual(nodes[0].id, expectedIds[0]);
+      assert.strictEqual(nodes[1].id, expectedIds[1]);
+      assert.strictEqual(nodes[2].id, expectedIds[2]);
+
+      // Validate Debts
+      assert.strictEqual(nodes[0].debt, expectedDebts[0]);
+      assert.strictEqual(nodes[1].debt, expectedDebts[1]);
+      assert.strictEqual(nodes[2].debt, expectedDebts[2]);
+
+      // Validate Denom
+      assert.strictEqual(nodes[0].denom, expectedDenom);
+      assert.strictEqual(nodes[1].denom, expectedDenom);
+      assert.strictEqual(nodes[2].denom, expectedDenom);
+
+      // Validate Balance
+      assert.strictEqual(nodes[0].balance, expectedBalance[0]);
+      assert.strictEqual(nodes[1].balance, expectedBalance[1]);
+      assert.strictEqual(nodes[2].balance, expectedBalance[2]);


by importing the strict module above, you don't have to repeat it here,

Suggested change

// Validate Ids

assert.strictEqual(nodes[0].id, expectedIds[0]);

assert.strictEqual(nodes[1].id, expectedIds[1]);

assert.strictEqual(nodes[2].id, expectedIds[2]);

// Validate Debts

assert.strictEqual(nodes[0].debt, expectedDebts[0]);

assert.strictEqual(nodes[1].debt, expectedDebts[1]);

assert.strictEqual(nodes[2].debt, expectedDebts[2]);

// Validate Denom

assert.strictEqual(nodes[0].denom, expectedDenom);

assert.strictEqual(nodes[1].denom, expectedDenom);

assert.strictEqual(nodes[2].denom, expectedDenom);

// Validate Balance

assert.strictEqual(nodes[0].balance, expectedBalance[0]);

assert.strictEqual(nodes[1].balance, expectedBalance[1]);

assert.strictEqual(nodes[2].balance, expectedBalance[2]);

assert.equal(nodes[0].id, expectedIds[0]);

assert.equal(nodes[1].id, expectedIds[1]);

assert.equal(nodes[2].id, expectedIds[2]);

but these lines are pretty verbose and you have to update them if you change the nodes or expectations. once you've made the change to take a single expectations parameter you can do,

Suggested change

// Validate Ids

assert.strictEqual(nodes[0].id, expectedIds[0]);

assert.strictEqual(nodes[1].id, expectedIds[1]);

assert.strictEqual(nodes[2].id, expectedIds[2]);

// Validate Debts

assert.strictEqual(nodes[0].debt, expectedDebts[0]);

assert.strictEqual(nodes[1].debt, expectedDebts[1]);

assert.strictEqual(nodes[2].debt, expectedDebts[2]);

// Validate Denom

assert.strictEqual(nodes[0].denom, expectedDenom);

assert.strictEqual(nodes[1].denom, expectedDenom);

assert.strictEqual(nodes[2].denom, expectedDenom);

// Validate Balance

assert.strictEqual(nodes[0].balance, expectedBalance[0]);

assert.strictEqual(nodes[1].balance, expectedBalance[1]);

assert.strictEqual(nodes[2].balance, expectedBalance[2]);

for (const key of Object.keys(expectations) {

for (let i = 0; i < nodes.length; i++) {

assert.equal(nodes[i][key], expecations[i].key);

}

}

Oh though you'll also have to take the plural s off the expectation fixture.

turadg · 2024-12-18T15:09:29Z

.github/workflows/liquidation.yml

+          expectedVaults: 10
+
+      - name: Place bid for 90IST
+        run: node .github/scripts/placeBid.mjs


Once you add the shebang you don't have to specify the interpreter at the callsite. It shouldn't be the concern of the caller how to run it.

Suggested change

run: node .github/scripts/placeBid.mjs

run: .github/scripts/placeBid.mjs

Separately, these scripts have nothing to do with Github. Please put them somewhere else in the repo. e.g. /scripts.

Suggested change

run: node .github/scripts/placeBid.mjs

run: ./scripts/placeBid.mjs

turadg · 2024-12-18T15:11:39Z

.github/scripts/changePrice.mjs

+if (!amount || !containerName || !agoricNet) {
+  console.error('Error: Missing one or more required parameters:');
+  if (!amount) console.error('Missing amount');
+  if (!containerName) console.error('Missing containerName');
+  if (!agoricNet) console.error('Missing agoricNet');
+  process.exit(1);
+}


if this script can import @agoric/internal you could do:

Suggested change

if (!amount || !containerName || !agoricNet) {

console.error('Error: Missing one or more required parameters:');

if (!amount) console.error('Missing amount');

if (!containerName) console.error('Missing containerName');

if (!agoricNet) console.error('Missing agoricNet');

process.exit(1);

}

assertAllDefined({ agoricNet, amount, containerName });

rabi-siddique · 2024-12-19T05:23:51Z

Thank you for the valuable suggestions. 🙌 I've made the changes.

turadg

I've made the changes

👍

rabi-siddique changed the base branch from main to rs-test December 17, 2024 09:00

rabi-siddique force-pushed the rs-automate-liquidation-testing branch 29 times, most recently from 6ebd1d1 to 5a92a43 Compare December 17, 2024 13:02

rabi-siddique force-pushed the rs-automate-liquidation-testing branch 7 times, most recently from 66fa359 to ed002c6 Compare December 18, 2024 13:15

rabi-siddique requested review from turadg and Muneeb147 December 18, 2024 13:16

rabi-siddique force-pushed the rs-automate-liquidation-testing branch 2 times, most recently from 51ed6c1 to fccc9a0 Compare December 18, 2024 13:37

rabi-siddique mentioned this pull request Dec 18, 2024

Remove encoding/decoding and update tests #35

Merged

turadg approved these changes Dec 18, 2024

View reviewed changes

rabi-siddique changed the base branch from rs-test to main December 19, 2024 07:00

rabi-siddique changed the base branch from main to rs-test December 19, 2024 07:05

rabi-siddique added 10 commits December 19, 2024 12:28

ci: initial setup for liquidation testing

43e2e7b

ci: set ATOM price and create vaults

8ed0d81

ci: place bids and set ATOM price to 9.99

998e5b4

ci: get active vaults

69a9a82

ci: validate liquidating vaults

5dd221d

fixup! ci: validate liquidating vaults

88ebe86

ci: set execute permission for all .mjs files in scripts

2390c50

chore(deps): install ses

18aafcf

fixup! chore(deps): install ses

9198abf

fixup! fixup! chore(deps): install ses

0df995d

rabi-siddique force-pushed the rs-automate-liquidation-testing branch from 7dc8b52 to 0df995d Compare December 19, 2024 07:28

turadg approved these changes Dec 19, 2024

View reviewed changes

rabi-siddique merged commit c912d4f into rs-test Dec 20, 2024
2 checks passed

rabi-siddique deleted the rs-automate-liquidation-testing branch December 20, 2024 03:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

automate liquidation testing #36

automate liquidation testing #36

rabi-siddique commented Dec 17, 2024 •

edited

Loading

turadg left a comment

turadg Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

turadg Dec 18, 2024

rabi-siddique commented Dec 19, 2024

turadg left a comment

	import { execa } from 'execa';
	#! /usr/bin/env node
	import { execa } from 'execa';

	import assert from 'assert';
	import assert from 'node:assert/strict';

		@@ -0,0 +1,157 @@
		// @ts-check
		import fetch from 'node-fetch';

	run: node .github/scripts/placeBid.mjs
	run: .github/scripts/placeBid.mjs

	run: node .github/scripts/placeBid.mjs
	run: ./scripts/placeBid.mjs

automate liquidation testing #36

automate liquidation testing #36

Conversation

rabi-siddique commented Dec 17, 2024 • edited Loading

turadg 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

rabi-siddique commented Dec 19, 2024

turadg left a comment

Choose a reason for hiding this comment

rabi-siddique commented Dec 17, 2024 •

edited

Loading