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

AssetHub: Atomic Swap NFT indexer M1 #1198

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

vikiival
Copy link
Contributor

@vikiival vikiival commented Jul 24, 2024

Milestone Delivery Checklist

  • The milestone-delivery-template.md has been copied and updated.
  • This pull request is being made by the same account as the accepted application.
  • I have disclosed any and all sources of reused code in the submitted repositories and have done my due diligence to meet its license requirements.
  • In case of acceptance, invoices must be submitted and payments will be transferred to the Polkadot AssetHub and/or fiat account provided in the application.
  • The delivery is according to the Guidelines for Milestone Deliverables.

Link to the application pull request: w3f/Grants-Program#2271 < please fill this in with the PR number of your application.

@vikiival vikiival changed the title Atomic Swap NFT indexer M1 AssetHub: Atomic Swap NFT indexer M1 Jul 24, 2024
@keeganquigley keeganquigley self-assigned this Jul 26, 2024
Copy link
Contributor

@keeganquigley keeganquigley left a comment

Choose a reason for hiding this comment

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

Hi @vikiival thanks for the delivery.

@vikiival
Copy link
Contributor Author

  • The license file is broken. Can you please fix it to point to

Fixed in 9a9ef8d

  • Are there unit tests for the CRUD handler as well?

Unit tests that require usage database are very tricky to write.
We have written unit tests to ensure that data transformation pipeline is returning expected data.

Regarding to the testing @subsquid team has promised some kind of testing framework for internals.

Screenshot 2024-07-29 at 11 01 16

Screenshot 2024-07-29 at 11 03 30

Copy link
Contributor

@keeganquigley keeganquigley left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @vikiival much appreciated. Moving forward, there are still some TODO comments in this file. Since this is the last milestone, are you able to address these now?

@keeganquigley
Copy link
Contributor

Regarding to the testing https://github.com/subsquid team has promised some kind of testing framework for internals.

Additionally, regarding the above, did you get a response?

@vikiival
Copy link
Contributor Author

did you get a response?

image

@vikiival
Copy link
Contributor Author

vikiival commented Aug 2, 2024

TODO comments in this file.

@vikiival vikiival requested a review from keeganquigley August 2, 2024 12:23
@vikiival
Copy link
Contributor Author

gm @keeganquigley any progress>

@keeganquigley
Copy link
Contributor

Thanks for the fixes @vikiival and sorry for the delay, as I was out of office last week. The TODOs have been fixed, and I'm assuming it will take some time for Subsquid to flesh out the internal testing framework (regarding the above). Since the application deliverables don't specify anything about testing for downstream dependencies, I'm willing to go ahead and pass it, but consider adding additional coverage.

Thanks and here is my final evaluation.

@keeganquigley keeganquigley merged commit 08b330d into w3f:master Aug 14, 2024
3 checks passed
Copy link

🪙 Please fill out the invoice form in order to initiate the payment process. Thank you!

@vikiival
Copy link
Contributor Author

sorry for the delay

No problem, I'd rather asked ^-^

as I was out of office last week.

Sun is shining ☀️, you are making awesome job. Your OOO was well-deserved!

@vikiival
Copy link
Contributor Author

Screenshot 2024-08-15 at 16 24 00

Form submitted.
Wish you a pleasant summer 🥹💚

anime-hot

@RouvenP
Copy link

RouvenP commented Aug 30, 2024

hi @vikiival we just transferred the DOTs

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.

3 participants