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

avoid storing redundant tx from blockpropose event #716

Merged
merged 6 commits into from
Jul 26, 2022
Merged

avoid storing redundant tx from blockpropose event #716

merged 6 commits into from
Jul 26, 2022

Conversation

LijuJoseJJ
Copy link
Contributor

@LijuJoseJJ LijuJoseJJ commented Jun 2, 2022 •
edited
Loading

this PR for now fixes wallet/browser and nightfall_client

  • only storing relevant transaction
  • also only storing block object for which any transaction is stored to db

fixes #667

@LijuJoseJJ LijuJoseJJ added the DNM Do not merge label Jun 2, 2022
@LijuJoseJJ LijuJoseJJ requested a review from IlyasRidhuan June 2, 2022 20:23
@LijuJoseJJ LijuJoseJJ removed the DNM Do not merge label Jun 5, 2022
@LijuJoseJJ LijuJoseJJ changed the title (wallet) avoid storing redundant tx from blockpropose event avoid storing redundant tx from blockpropose event Jun 13, 2022
druiz0992
druiz0992 suggested changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@druiz0992 druiz0992 left a comment

Choose a reason for hiding this comment

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

You will need to rebase with master and failing tests should now pass

druiz0992
druiz0992 reviewed Jun 17, 2022
View reviewed changes
Copy link
Contributor

@druiz0992 druiz0992 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 apply these changes to client-nightfall?

@LijuJoseJJ
Copy link
Contributor Author

Do we need to apply these changes to client-nightfall?

yes I believe, that why I refactored client as well in this PR

druiz0992
druiz0992 reviewed Jun 17, 2022
View reviewed changes
Copy link
Contributor

@druiz0992 druiz0992 left a comment

Choose a reason for hiding this comment

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

Can you think of how we can test this?

@LijuJoseJJ
Copy link
Contributor Author

LijuJoseJJ commented Jun 17, 2022 •
edited
Loading

Can you think of how we can test this?

the best way I could thing of (and the way i did is) to customise erc20.test.mjs for all scenarios, I also run wallet app for case where client is not hosting a particular user (i.e keys are not store in client).

@LijuJoseJJ LijuJoseJJ requested a review from druiz0992 June 17, 2022 13:00
@israelboudoux
Copy link
Contributor

Hey @LijuJoseJJ, are you still working on this PR?

@LijuJoseJJ
Copy link
Contributor Author

Hey @LijuJoseJJ, are you still working on this PR?

didn't notice merge conflict, will fix by tomorrow

@druiz0992 druiz0992 added the One more approval needed One reviewer has approved this PR but another is needed label Jul 11, 2022
daveroga
daveroga approved these changes Jul 26, 2022
View reviewed changes
Copy link
Contributor

@daveroga daveroga left a comment

Choose a reason for hiding this comment

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

LGTM

@daveroga daveroga merged commit 1e33881 into master Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daveroga daveroga daveroga approved these changes

@druiz0992 druiz0992 druiz0992 approved these changes

@IlyasRidhuan IlyasRidhuan Awaiting requested review from IlyasRidhuan

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.

Wallet to stop saving transactions it doesn't care about.
5 participants
@LijuJoseJJ @israelboudoux @druiz0992 @daveroga @IlyasRidhuan