[node-rewards-program] task: add service to communicate with API #679

OlegMakarenko · 2022-06-10T17:40:39Z

What was the issue?

Node Rewards Program module doesn't have service to communicate with API.

What's the fix?

Added Node Rewards Program service.
Added prepareEnrollTransaction() which returns transfer transaction filled with an enrollment data.
Added checkEnrollmentAddress() which calls /enrollment/check/address/<address_str> and maps result to boolean.
Added checkEnrollmentStatus() which checks if a signer public key is enrolled in the current period. It fetches latest successful enrollments from /enrollment/successes/<public_key_str> and check wheter a recipient address is the current enrollment address.
Added getEnrollmentHash() which fetches codeword dependent hash from /codeword/<public_key_str>.
Added getNodePayouts() which fetches payout information for a single node from /node/<int:node_id>/payouts.
Added getNodeInfo() which fetches detailed information about a single node from /node/<node_id>.

add methods: prepareEnrollTransaction, checkEnrollmentAddress, checkEnrollmentStatus and getEnrollmentHash

yilmazbahadir · 2022-06-13T12:48:07Z

src/app/services/nodeRewardsProgram.service.js

+const NODE_REWARDS_PROGRAM_API_BASE_URL = 'https://supernodesapi.nem.io';
+
+/** Service to enroll in the Node Rewards Program and to get the status related information */
+class nodeRewardsProgram {


should we apply a PascalCase since it's a class? (Capitalize the first letter)

Oops, thanks!

yilmazbahadir · 2022-06-13T12:51:44Z

src/app/services/nodeRewardsProgram.service.js

+     * @param {string} mainPublicKey - Delegated harvesting public key.
+     * @param {string} host - IP or domain of the node.
+     * @param {string} enrollmentAddress - Node Rewards Program enrollment address.
+     * @param {string} [multisigAccountAddress] - Multisig account address. For multisig enrollment only.


why square brackets? [multisigAccountAddress]

This is an optional parameter, according to the JSDoc syntax.

yilmazbahadir · 2022-06-13T13:05:46Z

I think we should also add some unit tests as well.

yilmazbahadir · 2022-06-13T20:02:29Z

tests/specs/nodeRewardsProgram.spec.js

+    }));
+}
+
+describe('NodeRewardsProgram service tests', function() {


Have you run this test in the browser?

Hint: Open the file build/tests/start.html in the browser.

Thanks. Tests updated.

yilmazbahadir · 2022-06-13T20:03:25Z

tests/specs/nodeRewardsProgram.spec.js

+            expect(transaction.mosaics).toBe(null);
+            expect(transaction.message).toBe(expectedMessage);
+            expect(transaction.isMultisig).toBe(false);
+            done();


I think the function needs to take the done as a parameter? (in all of the tests)

And perhaps it's not mandatory to call done?

Seems like here the async tests doesn't work correctly without calling done().
Without done(), I got this warning: SPEC HAS NO EXPECTATIONS.

…ion test, register service, cleanup

yilmazbahadir · 2022-06-14T14:47:24Z

Are the tests passing in your local?

OlegMakarenko · 2022-06-14T15:05:18Z

Works fine on my local.

Did you try npm run build?

yilmazbahadir · 2022-06-14T15:08:54Z

I think I missed it this time, looks okay 👍

AnthonyLaw · 2022-06-15T06:19:44Z

src/app/services/nodeRewardsProgram.service.js

+    async prepareEnrollTransaction(mainPublicKey, host, enrollmentAddress, multisigAccountAddress) {
+        // Create a new transaction object
+        const transferTransaction = nem.model.objects.get('transferTransaction');
+
+        // Set enrollment address as recipient
+        transferTransaction.recipient = enrollmentAddress.toUpperCase().replace(/-/g, '');
+
+        // Set multisig, if selected
+        if (multisigAccountAddress) {
+            transferTransaction.isMultisig = true;
+            transferTransaction.multisigAccount = multisigAccountAddress.toUpperCase().replace(/-/g, '');
+        }        
+
+        // Set the message
+        const hash = await this.getCodewordHash(mainPublicKey);
+        transferTransaction.message = `enroll ${host} ${hash}`;
+
+        // Set no mosaics
+        transferTransaction.mosaics = null;
+
+        return nem.model.transactions.prepare('transferTransaction')(this.common, transferTransaction, this._Wallet.network);
+    }


I think we can keep this in the controller.
since this method didn't require fetch

what do you think?

@AnthonyLaw Can you elaborate on this? (Which class are you referring to and why?)

@AnthonyLaw Can you elaborate on this? (Which class are you referring to and why?)

sure, I mean prepareEnrollTransaction method, not class.

for my understanding, *.service.js should just make a request to the endpoint NODE_REWARDS_PROGRAM_API_BASE_URL.

but prepareEnrollTransaction just prepares the transaction object, it does not have any interaction with another endpoint.

so I think preparing transaction object should happen in nodeRewardsProgram.controller.

I think this is not the case.

for my understanding, *.service.js should just make a request to the endpoint NODE_REWARDS_PROGRAM_API_BASE_URL

Please see this answer => https://stackoverflow.com/a/27398868/3190131

And this => https://docs.angularjs.org/guide/services

…ethods, add unit tests

tests/specs/nodeRewardsProgram.spec.js

AnthonyLaw · 2022-06-17T12:09:14Z

tests/specs/nodeRewardsProgram.spec.js

+            const mainPublicKey = '00a30788dc1f042da959309639a884d8f6a87086cda10300d2a7c3a0e0891a4d';
+            const host = 'http://108.61.168.86:7890';
+            const enrollmentAddress = 'NCHESTYVD2P6P646AMY7WSNG73PCPZDUQNSD6JAK';
+            const expectedMessage = nem.utils.convert.utf8ToHex(`enroll ${host} hash`);
+            const transferTransactionType = 257;


this was duplicated in a few tests, I think we can create it as an object to share with other tests.

tests/specs/nodeRewardsProgram.spec.js

AnthonyLaw · 2022-06-17T16:30:11Z

tests/specs/nodeRewardsProgram.spec.js

+            done();
+        });
+
+        it('should return valid multisig enroll transaction', async (done) => {


For all unit tests, I drop should (action => verb)

~~should~~ returns

src/app/services/nodeRewardsProgram.service.js

tests/specs/nodeRewardsProgram.spec.js

Jaguar0625 · 2022-06-22T15:39:36Z

tests/specs/nodeRewardsProgram.spec.js

+        it('should fetch to be called with valid endpoint url when publicKey passed', async (done) => {
+            // Arrange:
+            const mockedFetch = mockFetch(null);
+            const baseUrl = NODE_REWARDS_PROGRAM_API_BASE_URL;
+            const publicKey = '00a30788dc1f042da959309639a884d8f6a87086cda10300d2a7c3a0e0891a4d';
+            const expectedEndpoint = `${baseUrl}/codeword/${publicKey}`;
+
+            // Act:
+            await NodeRewardsProgram.getCodewordHash(publicKey);
+
+            // Assert:
+            expect(mockedFetch).toHaveBeenCalledWith(expectedEndpoint);
+            done()
+        });


i don't think this test make much sense as standalone.
instead, check that fetch was called in following two tests.

These three tests have different "object" of test. This test checks how the function cunstructs the request URL, but two other - a pass through and - the error to be thrown. Not sure, but it seems to me that if I remove 1st and add a fetch test to the 2nd and 3rd, it might be a "mixed" test. What do you think?

it depends how you define a unit. if you define a unit as something like "constructs url properly", "forwards object on success", then the separate tests make sense.

however, if you define a unit as a branch, like "handles successful fetch", "handles failed fetch", then combining makes sense. one benefit of this approach is that you will validate (in error cases) that the fetch was initiated (and that something previous didn't trigger the error).

defining units is more of an art than a science, so you can leave current cases if you prefer.

Thanks for the explanation, that makes sense.

Jaguar0625 · 2022-06-22T15:42:59Z

tests/specs/nodeRewardsProgram.spec.js

+            const expectedDTO = new Array(15).fill({
+                amount: 0,
+                fromRoundId: 844,
+                isPaid: false,
+                timestamp: '2022-06-16T12:15:27.427749+00:00',
+                toRoundId: 844,
+                transactionHash: null
+            });


probably better to return 15 unique objects vs 15 copies of same object

also, fyi - since function being tested is simply a pass through it's not required to match "real" shape from server, although that's find and there's nothing wrong with it (so you can leave it).

… remove unused variable

Jaguar0625 · 2022-06-22T20:27:02Z

tests/helper/index.js

+    // Act:
+    try {
+        await promiseToTest;
+    }


you should add assert here too to make sure test fails if exception isn't thrown

Updated. Moved assert from the catch section.

…rTest helper

* feat: init node rewards modules (#678) * feat: init node rewards modules * fix: clean up comment * [node-rewards-program] task: add service to communicate with API #679 * [node-rewards-program] task: add service add methods: prepareEnrollTransaction, checkEnrollmentAddress, checkEnrollmentStatus and getEnrollmentHash * [node-rewards-program] fix: typo in error message * [node-rewards-program] fix: typo in class name * [node-rewards-program] task: add tests * [node-rewards-program] fix: fetch mock, improve prepareEnrollTransaction test, register service, cleanup * [node-rewards-program] feat: add getNodePayouts() and getNodeInfo() methods, add unit tests * [node-rewards-program] fix: typo in jsdoc * [node-rewards-program] fix: test comments * [node-rewards-program] fix: remove whitespaces * [node-rewards-program] task: refactor fetch requests * [node-rewards-program] task: refactor the enrollment and error tests, remove unused variable * [node-rewards-program] task: update codeword test, fix runPromiseErrorTest helper * [node-rewards-program] task: add enrollment tab component (#681) * feat: add account selection section * build: add object-rest-spread plugins * feat: add node reward program alert message * [node-rewards-program] feat: add UI Tab component * [node-rewards-program] task: refactor UI and typo * [node-rewards-program] task: node reward controller * [node-rewards-program] task: add unit test * [node-rewards-program] task: fix typo * [node-rewards-program] task: fix send enrollment logic * [node-rewards-program] task: refactor on assign selected account * [node-rewards-program] task: refactor on tab and rename * [node-rewards-program] task: fix typo and lint * [node-rewards-program] task: change from toFixed to ceil * [node-rewards-program] task: minor refactor on unit test * Enhancement and feedback (#682) * [node-rewards-program] task: fix send enrollment logic * [node-rewards-program] task: fix typo and lint * [node-rewards-program] fix: change payout table width * [node-rewards-program] fix: format endpoint url to display only hostname * [node-rewards-program] feat: add last round payout property into Status tab * [node-rewards-program] fix: replace money bag icon * [node-rewards-program] perf: add check required params before calling API services * [node-rewards-program] feat: add validation for codeword hash * [node-rewards-program] fix: assign public key for new created account * [node-rewards-program] fix: allow update domain name when account enrolled * [node-rewards-program] task: enable hardware wallet enrollment * [node-rewards-program] task: fix typo * [node-rewards-program] task: rename module to superNode program (#683) * [node-rewards-program] bug: fix minor bug and refactor. (#684) * [node-rewards-program] task: add count params in getNodePayouts * [node-rewards-program] bug: fix unable send enrollement tx to update domain name * [node-rewards-program] task: removed unused method * [node-rewards-program] task: rename enroll in program and form data node host * [node-rewards-program] task: refactor on the logic * [node-rewards-program] task: new validation alert for node host * [node-rewards-program] task: update invalid node host message * [node-rewards-program] task: duplicated lastPayoutRound in unit test * [node-rewards-program] task: patch fix handle lastPayoutRound in null * [node-rewards-program] task: add translation (#685) * [node-rewards-program] task: add chinese translation * [node-rewards-program] task: add spanish translation * [node-rewards-program] task: add italian translation * [node-rewards-program] task: add japanese translation * [node-rewards-program] task: add polish translation * [node-rewards-program] task: add russian translation * [node-rewards-program] task: patch spanish translation * [node-rewards-program] task: add ukrainian translation * [node-rewards-program] task: patch italian translation * [node-rewards-program] task: add default translation in German, Nederlands and Portuguese * [node-rewards-program] task: patch Polish translation * [node-rewards-program] task: patch latest supernode program url and api endpoint * [node-rewards-program] task: patch minor fix Co-authored-by: OlegMakarenko <[email protected]>

OlegMakarenko added 2 commits June 10, 2022 20:26

[node-rewards-program] task: add service

153f5d8

add methods: prepareEnrollTransaction, checkEnrollmentAddress, checkEnrollmentStatus and getEnrollmentHash

[node-rewards-program] fix: typo in error message

2abefc0

OlegMakarenko linked an issue Jun 10, 2022 that may be closed by this pull request

NEM node rewards program support #677

Open

yilmazbahadir reviewed Jun 13, 2022

View reviewed changes

OlegMakarenko added 2 commits June 13, 2022 18:39

[node-rewards-program] fix: typo in class name

53fe266

[node-rewards-program] task: add tests

040a98e

yilmazbahadir reviewed Jun 13, 2022

View reviewed changes

[node-rewards-program] fix: fetch mock, improve prepareEnrollTransact…

763769e

…ion test, register service, cleanup

AnthonyLaw reviewed Jun 15, 2022

View reviewed changes

OlegMakarenko added 2 commits June 16, 2022 15:28

[node-rewards-program] feat: add getNodePayouts() and getNodeInfo() m…

e08e6c5

…ethods, add unit tests

[node-rewards-program] fix: typo in jsdoc

1f1ba29

AnthonyLaw reviewed Jun 17, 2022

View reviewed changes

tests/specs/nodeRewardsProgram.spec.js Show resolved Hide resolved

AnthonyLaw reviewed Jun 17, 2022

View reviewed changes

tests/specs/nodeRewardsProgram.spec.js Outdated Show resolved Hide resolved

AnthonyLaw reviewed Jun 17, 2022

View reviewed changes

tests/specs/nodeRewardsProgram.spec.js Outdated Show resolved Hide resolved

AnthonyLaw reviewed Jun 17, 2022

View reviewed changes

src/app/services/nodeRewardsProgram.service.js Show resolved Hide resolved

AnthonyLaw requested a review from Jaguar0625 June 17, 2022 17:00

[node-rewards-program] fix: test comments

cd63ec3

Jaguar0625 reviewed Jun 22, 2022

View reviewed changes

OlegMakarenko added 3 commits June 22, 2022 20:01

[node-rewards-program] fix: remove whitespaces

6cef4de

[node-rewards-program] task: refactor fetch requests

90991f3

[node-rewards-program] task: refactor the enrollment and error tests,…

75927ff

… remove unused variable

Jaguar0625 reviewed Jun 22, 2022

View reviewed changes

[node-rewards-program] task: update codeword test, fix runPromiseErro…

d9baab0

…rTest helper

Jaguar0625 approved these changes Jun 27, 2022

View reviewed changes

Jaguar0625 requested a review from 0x6861746366574 June 27, 2022 14:13

AnthonyLaw merged commit a63f24b into node-rewards-program Jun 28, 2022

AnthonyLaw deleted the feat/init-node-rewards-program-service branch June 28, 2022 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[node-rewards-program] task: add service to communicate with API #679

[node-rewards-program] task: add service to communicate with API #679

OlegMakarenko commented Jun 10, 2022 •

edited

Loading

yilmazbahadir Jun 13, 2022

OlegMakarenko Jun 13, 2022

OlegMakarenko Jun 13, 2022

yilmazbahadir Jun 13, 2022

OlegMakarenko Jun 13, 2022

yilmazbahadir commented Jun 13, 2022

yilmazbahadir Jun 13, 2022

OlegMakarenko Jun 14, 2022

yilmazbahadir Jun 13, 2022

yilmazbahadir Jun 13, 2022

OlegMakarenko Jun 14, 2022 •

edited

Loading

yilmazbahadir commented Jun 14, 2022

OlegMakarenko commented Jun 14, 2022

yilmazbahadir commented Jun 14, 2022

AnthonyLaw Jun 15, 2022

yilmazbahadir Jun 15, 2022

AnthonyLaw Jun 15, 2022

yilmazbahadir Jun 15, 2022

AnthonyLaw Jun 17, 2022

AnthonyLaw Jun 17, 2022

Jaguar0625 Jun 22, 2022

OlegMakarenko Jun 22, 2022

Jaguar0625 Jun 22, 2022

OlegMakarenko Jun 23, 2022

Jaguar0625 Jun 22, 2022

Jaguar0625 Jun 22, 2022

Jaguar0625 Jun 22, 2022

OlegMakarenko Jun 23, 2022

[node-rewards-program] task: add service to communicate with API #679

[node-rewards-program] task: add service to communicate with API #679

Conversation

OlegMakarenko commented Jun 10, 2022 • edited Loading

What was the issue?

What's the fix?

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

yilmazbahadir commented Jun 13, 2022

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

OlegMakarenko Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

yilmazbahadir commented Jun 14, 2022

OlegMakarenko commented Jun 14, 2022

yilmazbahadir commented Jun 14, 2022

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

OlegMakarenko commented Jun 10, 2022 •

edited

Loading

OlegMakarenko Jun 14, 2022 •

edited

Loading