Allow integer `revision` in `TypedData.Domain` #449

DelevoXDG · 2024-03-21T12:33:25Z

Describe your changes

TypedData.Domain can now be decoded from the following json, where revision is an integer:

"domain": {
    "name": "Starknet Example",
    "version": "1",
    "chainId": "SN_MAIN",
    "revision" : 1
  },

Add domain v0 object with string revision in TypedDataTest

Linked issues

Closes #448

Breaking changes

This issue contains breaking changes

TypedData.Domain.Revision is now of type JsonPrimitive? instead of TypedData.Revision?. Use TypedData.Domain.resolvedRevision to access revision as TypedData.Revision

- Convert `revision` from `TypedData.Revision` to `JsonPrimitive` - Add `resolvedRevision` val that parses `TypedData.Revision` from both strings and integers - Use `resolvedRevision` instead of `revision` where applicable

codecov-commenter · 2024-03-21T12:38:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.26%. Comparing base (d28b008) to head (ad53b98).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #449      +/-   ##
==========================================
- Coverage   71.12%   69.26%   -1.86%     
==========================================
  Files          75       75              
  Lines        3338     3254      -84     
  Branches      367      361       -6     
==========================================
- Hits         2374     2254     -120     
- Misses        798      845      +47     
+ Partials      166      155      -11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

DelevoXDG · 2024-03-21T12:38:48Z

Even though this change is not breaking for typed data where revision is a string, I suppose it's best we wait for starknet-io/SNIPs#79 to be merged.

franciszekjob · 2024-05-06T12:56:57Z

lib/src/main/kotlin/com/swmansion/starknet/data/TypedData.kt

+        @Transient
+        val resolvedRevision = revision?.let {
+            // Allow both string and integer values for revision
+            lenientJson.decodeFromJsonElement<Revision>(it)


I wonder if usage of isLenient is necessary here. kotlinx.serialization properly understands and maps enum Revision value by the provided serial name. Then "1" and 1 will both be correctly interpreted as Revision.V1, without the need of lenient mode.

Then instead of lenientJson.decodeFromJsonElement<Revision>(it) we can use
Json.decodeFromJsonElement<Revision>(it) and get rid of companion object with private val lenientJson.

Just want to make clear, whether adopting a lenient mode is needed, because based on the lenient json spec

Removes JSON specification restriction (RFC-4627) and makes parser more liberal to the malformed input.

@THenry14 wdyt?

properly understands and maps enum Revision value by the provided serial name.

As far as I remember, it doesn't. But maybe decodeFromJsonElement is indeed able to handle that without lenient mode. In that case, it may be unnecessary as you suggest.

Removes JSON specification restriction (RFC-4627) and makes parser more liberal to the malformed input.

The intention behind lenient mode here was:

In lenient mode quoted boolean literals, and unquoted string literals are allowed.

Yes, decodeFromJsonElement is able to handle that. Let's remove lenientJson then, as we discussed offline.

… into chore/448-typed-data-int-revision

franciszekjob

LGTM

THenry14 · 2024-05-07T10:04:31Z

lib/src/test/kotlin/starknet/data/TypedDataTest.kt

@@ -548,7 +548,7 @@ internal class TypedDataTest {
                "name": "DomainV1",
                "version": "1",
                "chainId": "2137",
-                "revision": "1"


do we have a test with "1" as well? I suppose it may still be possible to have such response, right?

Correct, considering "1" may occur in response, then these tests should be also added.

I don't think "1" is possible in revision 1 per the starknet-io/SNIPs#79

~~Besides, this case is covered by revision 0 typed data tests anyway, so probably we don't need any additional tests~~

so it is possible to get a shortstring, but this is atm limited to revision 0 right? so either way we need some test that have the revision as string, although if the snip is to be believed this should only occur for rev 0.

Note: The value of the field revision is the integer 1 eventhough the type of the field is shortstring

I find this a bit confusing :D

but this is atm limited to revision 0 right?

Well yeah, but also:

revision (optional): the revision of the specification to be used. If the value is omitted it will default to 0 .

Revision 0: Represents the de facto spec before this SNIP was published. The purpose is to help with backwards compatibility. It’s not recommended to use it.

We can add the test just for the sake of it, but we're testing something very unlikey

I find this a bit confusing :D

It is, but apparently handling shortstrings as ints or vice versa is a typical thing for typed data as I previously discovered trying to introduce some additional type checks which resulted in tests failing for existing examples

lib/src/test/kotlin/starknet/data/TypedDataTest.kt

Co-authored-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

DelevoXDG · 2024-05-10T12:43:24Z

lib/src/test/kotlin/starknet/data/TypedDataTest.kt

+        val domainTypeV1 = "StarknetDomain" to listOf(
+            TypedData.StandardType("name", "shortstring"),
+            TypedData.StandardType("version", "shortstring"),
+            TypedData.StandardType("chainId", "shortstring"),
+            TypedData.StandardType("revision", "shortstring"),
+        )


I think we can domain types from InvalidTypesTest to avoid duplication here.

I think we can't - domainTypeV1 used in InvalidTypesTest is accessible only in its scope, therefore it cannot be accessed in in domain v1 object with string revision test.

It's possible to create domainTypeV1 field directly in TypedDataTest, but I don't think its a good solution. Wdyt? @DelevoXDG

DelevoXDG · 2024-05-10T12:44:11Z

lib/src/test/kotlin/starknet/data/TypedDataTest.kt

+        val domainObjectV1WithStringRevision = """
+            {
+                "name": "DomainV1",
+                "version": "1",
+                "chainId": "2137",
+                "revision": "1"
+            }


Actually, let's make it domain v0 with revision. Because technically only v0 domain can have a string revision

…test

DelevoXDG

LGTM

DelevoXDG added 4 commits March 21, 2024 13:25

Update TypedData.Domain to allow integer revision

f04c8cb

- Convert `revision` from `TypedData.Revision` to `JsonPrimitive` - Add `resolvedRevision` val that parses `TypedData.Revision` from both strings and integers - Use `resolvedRevision` instead of `revision` where applicable

Update example files for tests

24018bb

Update domainObjectV1 in TypedDataTest in line with the adjustment

47b32cd

Merge branch 'main' into chore/448-typed-data-int-revision

f86864c

DelevoXDG added the blocked label Mar 21, 2024

DelevoXDG assigned DelevoXDG and unassigned DelevoXDG Mar 22, 2024

THenry14 removed the blocked label May 6, 2024

DelevoXDG requested a review from franciszekjob May 6, 2024 10:18

franciszekjob reviewed May 6, 2024

View reviewed changes

franciszekjob added 2 commits May 6, 2024 17:25

Merge branch 'main' of https://github.com/software-mansion/starknet-jvm…

6f997dc

… into chore/448-typed-data-int-revision

Remove lenientJson in Domain class

8b0f819

franciszekjob approved these changes May 6, 2024

View reviewed changes

THenry14 reviewed May 7, 2024

View reviewed changes

Add test for domain v1 revision as string and int

a96d0d9

THenry14 approved these changes May 10, 2024

View reviewed changes

Update tests naming

733b43c

franciszekjob reviewed May 10, 2024

View reviewed changes

lib/src/test/kotlin/starknet/data/TypedDataTest.kt Outdated Show resolved Hide resolved

lib/src/test/kotlin/starknet/data/TypedDataTest.kt Outdated Show resolved Hide resolved

DelevoXDG commented May 10, 2024

View reviewed changes

franciszekjob and others added 3 commits May 10, 2024 11:22

Update lib/src/test/kotlin/starknet/data/TypedDataTest.kt

4f2b20b

Co-authored-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

Update lib/src/test/kotlin/starknet/data/TypedDataTest.kt

8aa3e8c

Co-authored-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

Move domain v1 object with string revision to separate test

48a5125

DelevoXDG commented May 10, 2024

View reviewed changes

franciszekjob added 2 commits May 13, 2024 11:16

Update domain v0 object with string revision test

5d5bd64

Update typed data message in domain v0 object with string revision …

ad53b98

…test

DelevoXDG commented May 15, 2024

View reviewed changes

franciszekjob merged commit a3c8f8e into main May 15, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow integer `revision` in `TypedData.Domain` #449

Allow integer `revision` in `TypedData.Domain` #449

DelevoXDG commented Mar 21, 2024 •

edited by franciszekjob

codecov-commenter commented Mar 21, 2024 •

edited

DelevoXDG commented Mar 21, 2024 •

edited

franciszekjob May 6, 2024

DelevoXDG May 6, 2024 •

edited

franciszekjob May 6, 2024 •

edited

franciszekjob left a comment

THenry14 May 7, 2024

franciszekjob May 7, 2024

DelevoXDG May 10, 2024 •

edited

THenry14 May 10, 2024

DelevoXDG May 10, 2024

DelevoXDG May 10, 2024

franciszekjob May 10, 2024 •

edited

DelevoXDG May 10, 2024

DelevoXDG left a comment

Allow integer revision in TypedData.Domain #449

Allow integer revision in TypedData.Domain #449

Conversation

DelevoXDG commented Mar 21, 2024 • edited by franciszekjob

Describe your changes

Linked issues

Breaking changes

codecov-commenter commented Mar 21, 2024 • edited

Codecov Report

DelevoXDG commented Mar 21, 2024 • edited

franciszekjob May 6, 2024

Choose a reason for hiding this comment

DelevoXDG May 6, 2024 • edited

Choose a reason for hiding this comment

franciszekjob May 6, 2024 • edited

Choose a reason for hiding this comment

franciszekjob left a comment

Choose a reason for hiding this comment

THenry14 May 7, 2024

Choose a reason for hiding this comment

franciszekjob May 7, 2024

Choose a reason for hiding this comment

DelevoXDG May 10, 2024 • edited

Choose a reason for hiding this comment

THenry14 May 10, 2024

Choose a reason for hiding this comment

DelevoXDG May 10, 2024

Choose a reason for hiding this comment

DelevoXDG May 10, 2024

Choose a reason for hiding this comment

franciszekjob May 10, 2024 • edited

Choose a reason for hiding this comment

DelevoXDG May 10, 2024

Choose a reason for hiding this comment

DelevoXDG left a comment

Choose a reason for hiding this comment

Allow integer `revision` in `TypedData.Domain` #449

Allow integer `revision` in `TypedData.Domain` #449

DelevoXDG commented Mar 21, 2024 •

edited by franciszekjob

codecov-commenter commented Mar 21, 2024 •

edited

DelevoXDG commented Mar 21, 2024 •

edited

DelevoXDG May 6, 2024 •

edited

franciszekjob May 6, 2024 •

edited

DelevoXDG May 10, 2024 •

edited

franciszekjob May 10, 2024 •

edited