Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Save tx at transaction submit eventHandler for nightfall-client #1370

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Save tx at transaction submit eventHandler for nightfall-client #1370

wants to merge 9 commits into from

Conversation

LijuJoseJJ
Copy link
Contributor

@LijuJoseJJ LijuJoseJJ commented Feb 2, 2023 •
edited

What does this implement/fix? Explain your changes.

Changes in this PR includes

  1. implement transactionSubmittedEventHandler for nightfall-client
  2. save transaction in transactionSubmittedEventHandler if any of commitments and nullifiers are store in db for nightfall-client.
  3. update blockProposedEventHandler of nightfall-client to delete duplicate transactions and their commitments from db, if the transaction not in L2 i.e deleted commitments are not nullified on chain yet.

Does this close any currently open issues?

This PR fixes #1335

What commands can I run to test the change?

Any other comments?

This PR also implements github workflow with test filetest/client-resync.test.mjs, where user created two transfers transaction using same set of commitments, while proposer setMakeBlock was off. Note: second transfer is with higher fee.
then proposer starts making block pick second tx and make a successful block.
[to test client re-sync feature] we are restarts client and all unsaved transaction are saved at transactionSubmitEventHandler
and at blockProposedEventHandler second tx updates and first tx deleted with change commitment it created.

explanation on test/client-resync.test.mjs

steps are.

  1. do two deposit one by one each deposit test wait for TransactionSubmitted event and expect respective transaction hash and saved in client's db.
  2. create first transfer transaction, we also make sure it got stored in client database by waiting for TransactionSubmitted and after that querying client transaction collection via api.
  3. create second transfer transaction but this time we do not wait for TransactionSubmitted event also do not do makeBlock', and restart client immediatelyawait restartClient(nf3User);after client re-sync complete, check both transfer transactions in client db (this done at test by calling APIGET: /transactions`).
  4. after client re-sync and above check, start proposer make block await makeBlock();.
  5. At last check user balance also check user(client) transaction collections
    ( only three transaction should be in db, and last one should be of higher fee)

Westlad
Westlad requested changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@Westlad Westlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to include the Transaction events in the Client synchronisation routine now, otherwise it may not finish synchronisation with a consistent database?

@LijuJoseJJ LijuJoseJJ force-pushed the liju.jose/nightfall-client-saveTransaction-at-txSubmitEvent branch from d1ac399 to e2d769e Compare March 28, 2023 12:08
@LijuJoseJJ LijuJoseJJ requested a review from Westlad March 29, 2023 08:12
Westlad
Westlad requested changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@Westlad Westlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for your client synchronisation code (like the optimist resync test)

@LijuJoseJJ
Copy link
Contributor Author

Please add a test for your client synchronisation code (like the optimist resync test)

added

@LijuJoseJJ LijuJoseJJ requested a review from Westlad April 4, 2023 04:23
Westlad
Westlad requested changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@Westlad Westlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain how to test this code (including with a re-sync of the client).

@LijuJoseJJ
Copy link
Contributor Author

LijuJoseJJ commented Apr 19, 2023 •
edited

Please explain how to test this code (including with a re-sync of the client).

Thanks @Westlad for pointing it out.
I have updated description with the explanation of test/client-resync.test.mjs. (under Any other comments?)

@LijuJoseJJ LijuJoseJJ requested a review from Westlad April 19, 2023 08:54
@Westlad Westlad added the One more approval needed One reviewer has approved this PR but another is needed label Apr 19, 2023
@LijuJoseJJ LijuJoseJJ force-pushed the liju.jose/nightfall-client-saveTransaction-at-txSubmitEvent branch from d60303a to d3cb95f Compare May 10, 2023 11:23
@LijuJoseJJ LijuJoseJJ added the DNM Do not merge label May 10, 2023
@LijuJoseJJ LijuJoseJJ removed the DNM Do not merge label May 11, 2023
@LijuJoseJJ LijuJoseJJ requested a review from Westlad May 11, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IlyasRidhuan IlyasRidhuan Awaiting requested review from IlyasRidhuan

@Westlad Westlad Awaiting requested review from Westlad

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
One more approval needed One reviewer has approved this PR but another is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE] Client being able to re-use nullifiers that aren't still in a block
2 participants
@LijuJoseJJ @Westlad