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.

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

feat(unlock-app): Allow processing of large csvs #14055

Merged
merged 8 commits into from
Jun 19, 2024

Conversation

iMac7
Copy link
Contributor

@iMac7 iMac7 commented Jun 16, 2024

Description

Currently, only 50 entries are processed at a time and the user has to enter the csv again to process the next 50.
This PR loads the entire csv and queues the transactions into groups of 50 (maximum). Each group of transactions must be signed.

Issues

Fixes #12993

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the docs to reflect my changes if applicable
  • I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

Release Note Draft Snippet

Copy link

cla-bot bot commented Jun 16, 2024

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @iMac7 on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the file .clabot.
Thank you!

Copy link

cla-bot bot commented Jun 16, 2024

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @iMac7 on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the file .clabot.
Thank you!

@cla-bot cla-bot bot added the cla-signed label Jun 17, 2024
Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

LGTM!
Could you do a a small screen record with an example of how that behaves?

I wonder if we could just "skip" the wait once the tx has been submitted and just move on to the next one without having to wait for it to be mined?

@iMac7
Copy link
Contributor Author

iMac7 commented Jun 18, 2024

LGTM! Could you do a a small screen record with an example of how that behaves?

I wonder if we could just "skip" the wait once the tx has been submitted and just move on to the next one without having to wait for it to be mined?

https://www.loom.com/share/53879714d55747838de39c69161235e4

I used a MAX_SIZE of 1 when testing for convenience, so 4 values in the video = 4 signed transactions

@julien51
Copy link
Member

Thanks @iMac7 ! I believe it would be nice to add some kind of loading indicator to the UI, maybe replacing the the "drag and drop" area, showing that X transactions are being mined?

What do you think?

@iMac7
Copy link
Contributor Author

iMac7 commented Jun 18, 2024

Thanks @iMac7 ! I believe it would be nice to add some kind of loading indicator to the UI, maybe replacing the the "drag and drop" area, showing that X transactions are being mined?

What do you think?

Good idea, I'll take a look at that. The text that said the user should reupload a csv should be replaced as well, what do I put there?

@julien51
Copy link
Member

Good idea, I'll take a look at that. The text that said the user should reupload a csv should be replaced as well, what do I put there?

We should probably remove that text in fact and replace with something like this:

Due to block size limit, you can only airdrop at most 50 NFT per transaction, so if your list is longer than that, you will be prompted to approve multiple transactions. You can also easily restart the process at any point, as addresses who already have one NFT will be discard.

@julien51
Copy link
Member

@iMac7 Do you mind showing a quick video again with the updated loading state?

@iMac7
Copy link
Contributor Author

iMac7 commented Jun 19, 2024

@iMac7 Do you mind showing a quick video again with the updated loading state?

https://www.loom.com/share/8106d1ca03934774a71e92dc6f383042?sid=59ab7a74-2f3d-43ed-bce1-cf93c802b35d

I've not added number of transactions because they're processed in parallel anyway (lmk if the number is important)

@julien51
Copy link
Member

Amazing! LGTM!
Please add your wallet address to the issue so we can pay a bounty for this one!

@julien51 julien51 merged commit d27cb02 into unlock-protocol:master Jun 19, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate processing of large CSV files
2 participants