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

Calamar milestone 1 delivery #644

Merged
merged 2 commits into from
Dec 14, 2022
Merged

Conversation

uiii
Copy link
Contributor

@uiii uiii commented Dec 1, 2022

Milestone Delivery Checklist

Link to the application pull request: w3f/Grants-Program#1163


IMPORTANT we put a wrong BTC address in the application, the correct one is 32NoFbB4x8bZ7YNvjra1DUYcje2B2XQwP3 (is also mentioned in the invoice). Should we update it in the application as well? Fixed

@alxs alxs self-assigned this Dec 1, 2022
@alxs
Copy link
Contributor

alxs commented Dec 1, 2022

Hi @uiii, thank you for the delivery. I look forward to diving into it as soon as possible. Yes, in that case please update the BTC address in the application.

@alxs
Copy link
Contributor

alxs commented Dec 2, 2022

@uiii great work, thanks again for the excellent delivery. Everything is where it should be, the links to the examples are helpful to quickly verify the deliverables and the e2e testing and CI are looking great. I also love the UX/UI.

Out of all things, I seem to have trouble with the line endings when trying to build the Docker image: docker-build.txt. Could you look into it?

I also noticed some searches seem to time out.

If I had to pick something to improve, in the "parsed" parameter view I think (while much easier to read than the raw view) there is still room for improvement: for example, here the remark itself could take up more space and it's not clear what System and remark stand for or why the latter is listed twice. I would also add page numbers to list views. This is just personal feedback, feel free to ignore it.

By the way: If you want us to retweet an announcement linking to the article, feel free to send an email to grantsPR@web3.foundation.

@uiii
Copy link
Contributor Author

uiii commented Dec 2, 2022

@alxs great, thanks, like to hear you feedback.

Out of all things, I seem to have trouble with the line endings when trying to build the Docker image: docker-build.txt. Could you look into it?

I will fix it.

I also noticed some searches seem to time out.

Yes, we are aware of some cases where this doesn't work. This require the change of the backend system which is in control of Subsquid guy. We are discussing it with them, so we are definitely going to fix that, but it require more time to test new solutions and adapt our app to them.

the remark itself could take up more space

Idealy yes, but it is not easy to done, because I need to have the parsed view consisted for all the cases. It contains nested tables and I set the min-width of keys and values. It is compromise between the condensed view and the scrollbar. But I can think more about it. I thought about recalculating the sizes in runtime using javascript so it get the ideal layout.

it's not clear what System and remark stand for or why the latter is listed twice

It might be confusing, but it makes sense if you check the "raw" data. The System and first remark represnt the name of the call System.remark and the second remark is a key from the arguments to that call. I dont se a better way to display it (actually I do, but it requires to understand the semantics of the data, which is very complex thing).

I would also add page numbers to list views.

Yes good point. That kind of pagination remained here from times where we did not know the total counst of items.

By the way: If you want us to retweet an announcement linking to the article, feel free to send an email to grantsPR@web3.foundation.

Ah great. Can we (or others if retweeting) mention W3F grant in our tweets?

@uiii
Copy link
Contributor Author

uiii commented Dec 2, 2022

the fix for docker build is in master

@alxs
Copy link
Contributor

alxs commented Dec 6, 2022

Thanks a lot for your answers and for the fix. I confirm Docker is working now.

For deliverable 2, it seems that while both extrinsics and calls are displayed in a block's detail page, transfers are not. Could you add transfers as described in the application? What is the difference between calls and extrinsics, and is it really helpful to display both? The extrinsic detail page also seems to always link to a call with the same ID. This distinction doesn't exist for example on Subscan, where "Call" only represents the function that was executed.

By the way, I noticed accounts are currently displayed in hexadecimal instead of using the correct SS58 format. Do you want to implement this now, or were you planning to address it in a later milestone?

It might be confusing, but it makes sense if you check the "raw" data.

It might make sense if one checks the "raw" data, but it's confusing ;) In UX terms, I think it makes more sense to think about it this way. But I understand it may be a complex task and you have other things to focus on. Again, just feedback.

Ah great. Can we (or others if retweeting) mention W3F grant in our tweets?

Yes, but we ask teams to wait until the first milestone has been accepted. Please have a look at our Announcement Guidelines.

@uiii
Copy link
Contributor Author

uiii commented Dec 6, 2022

For deliverable 2, it seems that while both extrinsics and calls are displayed in a block's detail page, transfers are not.

It was meant block's extrinsics not transfers, I don't know why we wrote "tranfers" here 😕 We are not currently able to detect which extrinsic is transfer.

What is the difference between calls and extrinsics, and is it really helpful to display both? The extrinsic detail page also seems to always link to a call with the same ID. This distinction doesn't exist for example on Subscan, where "Call" only represents the function that was executed.

It was suggested by Subsquid as they have both entites in their archives. Call entity represents extrinsics action, there are cases where it is useful to have the distiction between the two, e.g. batch calls. See this extrinsic, it has Utility.batch call which has two child calls.

By the way, I noticed accounts are currently displayed in hexadecimal instead of using the correct SS58 format. Do you want to implement this now, or were you planning to address it in a later milestone?

Can you give me a link where it uses hex format, please? Because a I know we use SS58 encoding where possible

It might make sense if one checks the "raw" data, but it's confusing ;) In UX terms, I think it makes more sense to think about it this way. But I understand it may be a complex task and you have other things to focus on. Again, just feedback.

Yeah I understand it may be confusing, current implementation is the best we've come up with. Maybe we will find a better way later.

Yes, but we ask teams to wait until the first milestone has been accepted.

Yes, I will wait with it.

@uiii
Copy link
Contributor Author

uiii commented Dec 6, 2022

And the IDs (of extrinsic, calls, ...) are internal values from Subsquid, but they keep a specific pattern which can be mapped to be compatible with subscan, but it needs a little bit more research.

@alxs
Copy link
Contributor

alxs commented Dec 6, 2022

It was meant block's extrinsics not transfers, I don't know why we wrote "tranfers" here 😕 We are not currently able to detect which extrinsic is transfer.

No worries. If this is something you can't implement at the moment, could you please submit an amendment and move it to a different milestone (from the Account Detail section and the deliverables)?

It was suggested by Subsquid as they have both entites in their archives. Call entity represents extrinsics action, there are cases where it is useful to have the distiction between the two, e.g. batch calls.

Ah, of course! That makes sense. Nevertheless this seems useful for that specific case, but in the case of all other extrinsics or e.g. listing all extrinsics and calls in the block details page seems superfluous.

Can you give me a link where it uses hex format, please? Because a I know we use SS58 encoding where possible

It might only apply to specific pages then. I noticed it here (both for the account and the destination).

@uiii
Copy link
Contributor Author

uiii commented Dec 6, 2022

could you please submit an amendment and move it to a different milestone (from the Account Detail section and the deliverables)?

The block's "transfers" instead of "extrinisics" in the first milestone deliverable is more like a typo. The Account Detail describes the final application state and is part of the second milestone, so the transfers here are correct. But I can update the application and fix the typo.

Ah, of course! That makes sense. Nevertheless this seems useful for that specific case, but in the case of all other extrinsics or e.g. listing all extrinsics and calls in the block details page seems superfluous.

Yes it seems superfluous a little bit, I will think about it. Maybe we can have the call list in extrinsic's detail page only.

It might only apply to specific pages then. I noticed it here (both for the account and the destination).

Ah it was actually a bug, all the addresses in polkadot relay chain entities wasn't encoded, it is fixed in the master now and released into production.

@alxs
Copy link
Contributor

alxs commented Dec 12, 2022

Hey @uiii, sorry for the late reply. I have a bit of a backlog.

The Account Detail describes the final application state and is part of the second milestone, so the transfers here are correct. But I can update the application and fix the typo.

I see, so you will actually implement it in the next milestone for the Account Details view. Even so, I think it'd be good if you amended it for this milestone, since the deliverable in milestone 2 doesn't cover the same functionality. You can also change deliverable 8 so it matches the implementation, and make changes to the upcoming milestones if you need to. It shouldn't take long to be approved.

@uiii uiii mentioned this pull request Dec 12, 2022
3 tasks
@uiii
Copy link
Contributor Author

uiii commented Dec 12, 2022

All right @alxs, I made an amendment.

@alxs
Copy link
Contributor

alxs commented Dec 14, 2022

Thank you. With this, I'm happy to let you know that the milestone has been accepted! You can find my evaluation notes here.

I'll notify the operations team to pay out your invoice. Please allow for up to 14 days for processing.

@alxs alxs merged commit d4af66c into w3f:master Dec 14, 2022
@github-actions
Copy link

Congratulations on completing the first milestone of this grant! As part of the Grants Program, we want to help grant recipients acknowledge their grants publicly. To that end, we’ve created a badge for projects that successfully deliver their first milestone. Note that it must only be used within the context of the delivered work, so please do not display it on your team or project's homepage unless accompanied by a short description of the grant.

Furthermore, you're now welcome to announce the grant publicly. Please remember to observe the foundation’s guidelines in doing so. In case you haven't done so yet, you may also reach out to grantsPR@web3.foundation for feedback on your announcement and cross-promotion.

Thank you for your contribution and good luck with the remaining milestones, if any! As usual, please let us know if you run into any delays by leaving a comment on the application PR, or directly submitting an amendment.

@uiii
Copy link
Contributor Author

uiii commented Dec 16, 2022

Great @alxs, I'm happy to hear that. Thanks

@RouvenP
Copy link

RouvenP commented Dec 21, 2022

hi @uiii we transferred the payment yesterday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants