Luizoamorim/feat save commitments #867

luizoamorim · 2022-08-10T13:28:13Z

This PR add an endpoint to saveCommitments functionality and also a function in the service to do the data manupulation.

…om endpoint saveall

nightfall-client/src/services/commitment-storage.mjs

imagobea

Hello @luizoamorim

Apologies for the late feedback. As discussed:

We have implemented a number of validations in the SDK, but perhaps they belong in the API in case other requests are sent from different services
In standup today we confirmed that an invalid commitment can't be spent, this means we might want to bulk insert all commitments (after proving that the "format" of the commitment is correct). At the same time, I'm not familiar with MongoDB, so might be good to check what would happen in the event of trying to insert an existing id.
Once we have addressed the above and the logic is clear, we should double-check the route (we prob don't need lines 103 - 105)

Many thanks ☮️

- also added the export for syncState function to be used in the import commitments.

luizoamorim · 2022-08-20T11:55:35Z

@daveroga @imagobea @israelboudoux

I decided to follow the logic of remove the existent commitments found in database. The other using upsert I didn't had success.
So I'm following the next steps:

/**
   * 1. listOfCommitments => get only the ids
   */
  const commitmentsIds = listOfCommitments.map(commitment => commitment._id);
  /**
   * 2. Find commitments that already exists in DB
   */
  const commitmentsFound = await db
    .collection(COMMITMENTS_COLLECTION)
    .find({ _id: { $in: commitmentsIds } })
    .toArray();

  /**
   * 3. remove the commitments found in the database from the list
   */
  const onlyNewCommitments = listOfCommitments.filter(
    commitments =>
      commitmentsFound.find(commitmentFound => commitmentFound.id === commitments.id) === undefined,
  );

  if (onlyNewCommitments.length > 0) {
    /**
     * 4. Insert all
     */
    await db.collection(COMMITMENTS_COLLECTION).insertMany(onlyNewCommitments);

    /**
     * 5. Sycronize from beggining
     */
    await syncState();

    return { successMessage: 'Commitments have been saved successfully!' };

imagobea

Very good @luizoamorim 💃
I have added some thoughts and recommended some changes, but to me this is exactly what we agreed. Well played!! 🪘

imagobea · 2022-08-22T08:09:58Z

nightfall-client/src/event-handlers/block-proposed.mjs

@@ -1,3 +1,4 @@
+/* eslint-disable import/no-cycle */


👀 I can see this rule has been added to several files in the PR, is this needed? What was the error??

This problem started to appear. I don't know exactly why.

imagobea · 2022-08-22T08:12:05Z

nightfall-client/src/services/commitment-storage.mjs

@@ -753,14 +755,61 @@ export async function findUsableCommitmentsMutex(
  );
 }

+/**
+ *
+ * @function saveCommitments save a list of commitments in the database


I suggest to rename the function as e.g. insertCommitmentsAndResync (?)

Can be. Changed.

imagobea · 2022-08-22T08:13:12Z

nightfall-client/src/services/commitment-storage.mjs

+ * @returns if all the commitments in the list already exists in the database
+ * throw an error, else return a success message.


You can use the @throws {Error} to explain the "throw error" bit

imagobea · 2022-08-22T08:14:14Z

nightfall-client/src/services/commitment-storage.mjs

+  /**
+   * 1. listOfCommitments => get only the ids
+   */


Make the comments less verbose

Suggested change

/**

* 1. listOfCommitments => get only the ids

*/

// listOfCommitments => get only the ids

Changed for just 1 line.

imagobea · 2022-08-22T08:14:44Z

nightfall-client/src/services/commitment-storage.mjs

+  /**
+   * 1. listOfCommitments => get only the ids
+   */
+  const commitmentsIds = listOfCommitments.map(commitment => commitment._id);


Suggested change

const commitmentsIds = listOfCommitments.map(commitment => commitment._id);

const commitmentIds = listOfCommitments.map(commitment => commitment._id);

In this case I prefer leave in plural. Because a list of ids for several commitments not just one.

imagobea · 2022-08-22T08:17:20Z

nightfall-client/src/services/commitment-storage.mjs

+  /**
+   * 2. Find commitments that already exists in DB
+   */
+  const commitmentsFound = await db


Suggested change

const commitmentsFound = await db

const commitmentsFromDb = await db

Make sense! Changed.

imagobea · 2022-08-22T08:20:21Z

nightfall-client/src/services/commitment-storage.mjs

+      commitmentsFound.find(commitmentFound => commitmentFound.id === commitments.id) === undefined,
+  );
+
+  if (onlyNewCommitments.length > 0) {


Length 0 is falsy already

Suggested change

if (onlyNewCommitments.length > 0) {

if (onlyNewCommitments.length) {

Can be! Changed!

imagobea · 2022-08-22T08:31:17Z

nightfall-client/src/services/commitment-storage.mjs

+  const onlyNewCommitments = listOfCommitments.filter(
+    commitments =>
+      commitmentsFound.find(commitmentFound => commitmentFound.id === commitments.id) === undefined,
+  );


Singular!
Very nice chain of array methods 😊 👏

Suggested change

const onlyNewCommitments = listOfCommitments.filter(

commitments =>

commitmentsFound.find(commitmentFound => commitmentFound.id === commitments.id) === undefined,

);

const onlyNewCommitments = listOfCommitments.filter(

commitment =>

commitmentsFound.find(commitmentFound => commitmentFound.id === commitment.id) === undefined,

);

imagobea · 2022-08-22T08:40:03Z

nightfall-client/src/routes/commitment.mjs

+router.post('/saveAll', async (req, res, next) => {
+  logger.debug('commitment/saveAll endpoint received POST');


If we had a standard API, POST /commitments would already signal insertion.

I assume the above is not possible, therefore perhaps a "one-word" route should suffice, e.g. /save. This could also be /insert or /create.

Changed to save

daveroga

LGTM

…e database changed commitment.id to commiment._id that is the correct field

We already applied the improvements.

luizoamorim added 2 commits August 5, 2022 09:41

chore: add endpoint that will receive a list of commitments to be saved

0b1ec08

chore: add a service that will save a list of commitments received fr…

c51fe77

…om endpoint saveall

luizoamorim requested review from imagobea and israelboudoux August 10, 2022 13:28

imagobea linked an issue Aug 10, 2022 that may be closed by this pull request

Add an endpoint to nightfall-client API for importing data #804

Closed

4 tasks

druiz0992 previously requested changes Aug 11, 2022

View reviewed changes

nightfall-client/src/services/commitment-storage.mjs Outdated Show resolved Hide resolved

Westlad added the Changes requested label Aug 12, 2022

imagobea suggested changes Aug 12, 2022

View reviewed changes

luizoamorim added 3 commits August 16, 2022 18:57

Merge branch 'master' into luizoamorim/feat-save-commitments

743ca30

chore: improvments for save all commitments endpoint and service

71d0a5b

- also added the export for syncState function to be used in the import commitments.

chore: solving import nocycle eslint problem for this files

bf698c6

luizoamorim requested review from imagobea and druiz0992 August 20, 2022 11:53

imagobea approved these changes Aug 22, 2022

View reviewed changes

imagobea added One more approval needed One reviewer has approved this PR but another is needed and removed Changes requested labels Aug 22, 2022

daveroga approved these changes Aug 22, 2022

View reviewed changes

luizoamorim added 2 commits August 22, 2022 22:23

refactor: applying pr reviews

78c7a96

fix(commitments): verification if already exist some commitment in th…

8c07cf4

…e database changed commitment.id to commiment._id that is the correct field

imagobea removed the One more approval needed One reviewer has approved this PR but another is needed label Aug 24, 2022

luizoamorim merged commit 7f722a2 into master Aug 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Luizoamorim/feat save commitments #867

Luizoamorim/feat save commitments #867

luizoamorim commented Aug 10, 2022

imagobea left a comment

luizoamorim commented Aug 20, 2022

imagobea left a comment

imagobea Aug 22, 2022

luizoamorim Aug 22, 2022

imagobea Aug 22, 2022

luizoamorim Aug 22, 2022

imagobea Aug 22, 2022

luizoamorim Aug 22, 2022

imagobea Aug 22, 2022

luizoamorim Aug 22, 2022

imagobea Aug 22, 2022

luizoamorim Aug 22, 2022

imagobea Aug 22, 2022

luizoamorim Aug 22, 2022

imagobea Aug 22, 2022

luizoamorim Aug 22, 2022

imagobea Aug 22, 2022

luizoamorim Aug 22, 2022

imagobea Aug 22, 2022

luizoamorim Aug 22, 2022

daveroga left a comment

		* @returns if all the commitments in the list already exists in the database
		* throw an error, else return a success message.

	const commitmentsIds = listOfCommitments.map(commitment => commitment._id);
	const commitmentIds = listOfCommitments.map(commitment => commitment._id);

	const commitmentsFound = await db
	const commitmentsFromDb = await db

	if (onlyNewCommitments.length > 0) {
	if (onlyNewCommitments.length) {

		router.post('/saveAll', async (req, res, next) => {
		logger.debug('commitment/saveAll endpoint received POST');

Luizoamorim/feat save commitments #867

Luizoamorim/feat save commitments #867

Conversation

luizoamorim commented Aug 10, 2022

imagobea left a comment

Choose a reason for hiding this comment

luizoamorim commented Aug 20, 2022

imagobea 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

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

daveroga left a comment

Choose a reason for hiding this comment