SmartTokenLabs / TokenScript

TokenScript schema, specs and paper
http://tokenscript.org
MIT License
242 stars 71 forks source link

Distinct #388

Closed darakhbharat closed 4 years ago

darakhbharat commented 4 years ago

Hi Weiwu,

All three issues are resolved and tested on multiple tokenscript xml files and found working with XSD 1.1.

If you review and verify can be merged into master, but not sure if there could be conflicts as this branch is 6 commit behind master.

I have my local Java setup for validating XSD 1.1.
As mentioned earlier XMLSECTOOL supports XSD 1.1 (used in validate.sh make file) but I am not able to install it on my machine. I have XMLLINT which does not support XSD 1.1. So I came up with Java(Xerces) set up.

It might be easier for any one of the colleague to check this out and enable XSD 1.1 and test these changes before merging. It might work already as well but need to give it a try.

If in any case we are unable to use XSD 1.1 we can still comment below two lines from tokenscript.xsd and merge the changes. As other changes are XSD 1 compliant and resolve the issue no. 3 from - https://github.com/AlphaWallet/TokenScript/issues/378

  1. distinct attribute is not allowed for attributes in , only if it is directly in (the global element ).
<assert test="count(ts:attribute[string(@distinct) = 'true']) le 1"/>
<assert test="if(not(ts:origins)) then count(ts:attribute[string(@distinct) = 'true']) = 1 else true()"/> 
SmartLayer commented 4 years ago

Thanks a lot! Notice that Tokenscript.xsd has changed since you forked it. Can you take a look if you can resolve the conflict yourself? I'm happy to help in case you have problem with that. I quickly checked and it seems the conflicts are simple.

I'll check this again tomorrow morning.

darakhbharat commented 4 years ago

Okay will check it out if I can resolve it.

On Thu, Sep 17, 2020 at 7:48 PM Weiwu Zhang notifications@github.com wrote:

Thanks a lot! Notice that Tokenscript.xsd has changed since you forked it. Can you take a look if you can resolve the conflict yourself? I'm happy to help in case you have problem with that. I quickly checked and it seems the conflicts are simple.

I'll check this again tomorrow morning.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/AlphaWallet/TokenScript/pull/388#issuecomment-694267621, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE47O3POWT4SZR4MOZN4TKDSGILBTANCNFSM4RQFXTBQ .

darakhbharat commented 4 years ago

Thanks a lot! Notice that Tokenscript.xsd has changed since you forked it. Can you take a look if you can resolve the conflict yourself? I'm happy to help in case you have problem with that. I quickly checked and it seems the conflicts are simple.

I'll check this again tomorrow morning.

I have resolved the conflicts and now it is ready for merging after your review. Basic testing is done after resolving conflicts and works fine as explained in main comment - https://github.com/AlphaWallet/TokenScript/pull/388/#issue-488551854

SmartLayer commented 4 years ago

Thank you so much @darakhbharat. The reason I didn't merge this one in despite you have tested the schema is that I found some other changes are needed (attribute mapping as an origin of attribute) in order to fully support distinct so I am adding these in as well.

darakhbharat commented 4 years ago

Okay. I had locally created all positive and negative test cases for all three use cases for the testing purpose. Let me know if there are other / any action items for me. Thanks!

On Tue, Sep 29, 2020 at 1:15 PM Weiwu Zhang notifications@github.com wrote:

Thank you so much @darakhbharat https://github.com/darakhbharat. The reason I didn't merge this one in despite you have tested the schema is that I found some other changes are needed (attribute mapping as an origin of attribute) in order to fully support distinct so I am adding these in as well.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AlphaWallet/TokenScript/pull/388#issuecomment-700662666, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE47O3JT4JH5UMOG6JIVMXLSIHFWHANCNFSM4RQFXTBQ .