SmartTokenLabs / TokenScript

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

numeric string type ASN.1 is not checksummed on address #357

Open bitcoinwarrior1 opened 4 years ago

bitcoinwarrior1 commented 4 years ago

observe the following:

<ts:selection id="notAdmin" filter="!(ownerAddress=admin)">
    <ts:denial>
        <ts:string xml:lang="en">Must be the admin</ts:string>
    </ts:denial>
</ts:selection>

With the following admin attribute:

<ts:attribute-type name="admin" syntax="1.3.6.1.4.1.1466.115.121.1.15">
    <ts:label>
        <ts:string xml:lang="en">admin</ts:string>
    </ts:label>
    <ts:origins>
        <ethereum:call function="admin" contract="GFO" as="address"/>
    </ts:origins>
</ts:attribute-type>

This comparison will not work because the attribute type admin is not checksummed but the owner address is, thereby making this selection impossible.

We need an ASN.1 syntax which handles checksums, not only for this use case but in general as it is unsafe NOT to use a checksum on an address.

SmartLayer commented 4 years ago

There are 2 ways to solve this problem:

𝑎) relax the filter expression rule implementation to allow 0x… integer 𝑏) introduce typing to attributes (therefore onboarding caseIgnoreMatch matching rule.)

The first, 𝑎) is a quick fix. Let's define the syntax of admin to be Integer ( 1.3.6.1.4.1.1466.115.121.1.27 ) and relax the search expression to allow hex, therefore an owner_address address 0xdecafbad can be used in the expression (!(ownerAddress=admin)).

The disadvantage is that this is not applicable to some other blockchains (e.g. Bitcoin), or, when Ethereum upgrades to include a new address coding method like Bitcoin did when it upgraded to the bech32 format (stuff like bc1qar0srrr7xfkvy5l643lydnw9re59gtzzwf5mdq). This will involve a change in every existing TokenScript where an attribute represents an address (not much) and make the expression evaluation bits in αW ios/Android flexible (which @JamesSmartCell is doing on his side already.)

The second 𝑏) is to implement the typing of attributes which was originally intended for schema/06 but missed the deadline. It will involve changing every existing TokenScript files but I'd rather do this in 2020/06 than in 2021/06 (this is not something I come up suddenly - I wanted to propose this change ever since Boon recommended renaming <attribute-type> to <attribute>.

https://community.tokenscript.org/t/attribute-typing/368

SmartLayer commented 4 years ago

What I propose is to do a full 𝑎 and a half-way 𝑏:

Convert this:

<ts:attribute-type name="admin" syntax="1.3.6.1.4.1.1466.115.121.1.15">
…

Into this:

<ts:attribute name="admin">
   <ts:type><ts:syntax>1.3.6.1.4.1.1466.115.121.1.27</ts:syntax><ts:type>
…

Rational:

  1. This avoids implementing <ts:equality>, therefore it's only a syntax change in the codebase, none of αW ios or android needs to change the way attributes are stored and handled. Easy to make it into 2020/06

  2. When some day we properly decided what to do with the typing of attribute, we don't have to force any change of existing tokenscript, only that the new tokenscript should have additional elements.

hboon commented 4 years ago

I think the original snippet which @James-Sangalli posted might already work in the iOS version. Does it, James?

In the iOS implementation, every (attribute) value, wherever possible, is already associated with a "type", be it the syntax or the as). We should have to do something like this in order to implement this anyway:

𝑏) introduce typing to attributes (therefore onboarding caseIgnoreMatch matching rule.)

There is this bit of code in iOS:

func isTrueFor(attributeValue: AssetInternalValue, value: String) -> Bool {
    switch attributeValue {
    case .address(let address):
        switch self {
        case .equal:
            return address.eip55String.lowercased() == value.lowercased()

Because the implicit value ownerAddress is injected/interpolated as an "address" (actually Ethereum address for now), the comparsion operator "=" compares them case-insensitively.

Would this be expected behavior as well as easier to implement right away and closer towards the path of (𝑏), if Android is already tagging types — without thinking and enforcing too much type conversion and without requiring any change to the TokenScript files and schema.

If so, that's my (c)

bitcoinwarrior1 commented 4 years ago

I think the original snippet which @James-Sangalli posted might already work in the iOS version. Does it, James?

In the iOS implementation, every (attribute) value, wherever possible, is already associated with a "type", be it the syntax or the as). We should have to do something like this in order to implement this anyway:

𝑏) introduce typing to attributes (therefore onboarding caseIgnoreMatch matching rule.)

There is this bit of code in iOS:

func isTrueFor(attributeValue: AssetInternalValue, value: String) -> Bool {
    switch attributeValue {
    case .address(let address):
        switch self {
        case .equal:
            return address.eip55String.lowercased() == value.lowercased()

Because the implicit value ownerAddress is injected/interpolated as an "address" (actually Ethereum address for now), the comparsion operator "=" compares them case-insensitively.

Would this be expected behavior as well as easier to implement right away and closer towards the path of (𝑏), if Android is already tagging types — without thinking and enforcing too much type conversion and without requiring any change to the TokenScript files and schema.

If so, that's my (c)

@hboon while that may be the case, the admin attribute doesn't resolve in time for the selection logic so it doesn't work right now. The admin address is resolved correctly and does do the checksum.

admin
bitcoinwarrior1 commented 4 years ago

I think the original snippet which @James-Sangalli posted might already work in the iOS version. Does it, James? In the iOS implementation, every (attribute) value, wherever possible, is already associated with a "type", be it the syntax or the as). We should have to do something like this in order to implement this anyway:

𝑏) introduce typing to attributes (therefore onboarding caseIgnoreMatch matching rule.)

There is this bit of code in iOS:

func isTrueFor(attributeValue: AssetInternalValue, value: String) -> Bool {
    switch attributeValue {
    case .address(let address):
        switch self {
        case .equal:
            return address.eip55String.lowercased() == value.lowercased()

Because the implicit value ownerAddress is injected/interpolated as an "address" (actually Ethereum address for now), the comparsion operator "=" compares them case-insensitively. Would this be expected behavior as well as easier to implement right away and closer towards the path of (𝑏), if Android is already tagging types — without thinking and enforcing too much type conversion and without requiring any change to the TokenScript files and schema. If so, that's my (c)

@hboon while that may be the case, the admin attribute doesn't resolve in time for the selection logic so it doesn't work right now. The admin address is resolved correctly and does do the checksum.

admin

@hboon flag this, it is probably me now that I think of it because the enabled selection resolves in compound