Rework overrided constructor with classmethod

[LeaveMyYard]

Overriding __init__ in pydantic's BaseModels is bad in a few reasons:

1. It breaks the Liskov Substitution Principle
2. Because of it it is impossible to use SiweMessage in FastAPI or other BaseModels

As a solution, I propose to move the custom __init__ to a classmethod.

[sbihel]

I realise I might have introduced pydantic incorrectly. Your mention of FastAPI makes me think you are using SIWE messages in an API as a JSON object instead of using the EIP-4361 serialisation format.

Should SiweMessage instead be a Custom Data Type? I am not entirely sure how to leverage pydantic for cleaner code, enforce string serialisation, and make construction easy. But with #39, it's the occasion to think about it.

[LeaveMyYard]

So, you say that I should not send this:

{
    "message": {
        "domain": "...",
        "address": "...",
        "statement": "...",
        "uri": "...",
        "version": "1",
        "chainId": 1,
        "nonce": "kjNBEqFTkPc",
        "issuedAt": "2023-08-09T11:48:36.694Z"
    },
    "signature": "..."
}

But this:

{
    "message": "<EIP-4361 serialized message>",
    "signature": "..."
}

---

In any case I think this is still better to have a classmethod. In case of FastAPI:

@app.post("/verify")
async def verify(message: str, signature: str) -> bool:
    message_object = SiweMessage.from_message(message)
    message_object.verify(signature, ...)
    ...

And then to use the object straight away it will just work:

@app.post("/verify")
async def verify(message: SiweMessage, signature: str) -> bool:
    message.verify(signature, ...)
    ...

But I still like passing the exact object more, as it is good both for OpenAPI and GraphQL, as they will have the exact object structure. Maybe I don't understand something, as I just discovered SIWE.

In any case I think that Pydantic usage here is good, you did wrote a good code overall.

[sbihel]

So, you say that I should not send this:

{
    "message": {
        "domain": "...",
        "address": "...",
        "statement": "...",
        "uri": "...",
        "version": "1",
        "chainId": 1,
        "nonce": "kjNBEqFTkPc",
        "issuedAt": "2023-08-09T11:48:36.694Z"
    },
    "signature": "..."
}

But this:

{
    "message": "<EIP-4361 serialized message>",
    "signature": "..."
}

Yes, because there's no other format defined in the specs. E.g. in the case of JSON it doesn't say if it should be camelCase or pascal_case.

In any case I think this is still better to have a classmethod. In case of FastAPI:

@app.post("/verify")
async def verify(message: str, signature: str) -> bool:
    message_object = SiweMessage.from_message(message)
    message_object.verify(signature, ...)
    ...

And then to use the object straight away it will just work:

@app.post("/verify")
async def verify(message: SiweMessage, signature: str) -> bool:
    message.verify(signature, ...)
    ...

But I still like passing the exact object more, as it is good both for OpenAPI and GraphQL, as they will have the exact object structure. Maybe I don't understand something, as I just discovered SIWE.

That's totally fair, but it's probably something you should define yourself, then the compatibility between frontend/backend will be ensured

[LeaveMyYard]

@ameyarao98 implemented removing dict annotation, now the code is indeed cleaner

[ameyarao98]

bump

[LeaveMyYard]

@ameyarao98 updated readme

[ameyarao98]

Looks good to me

[sbihel]

Closed by #59

[ameyarao98]

The linked PR does not fully fix this.

1. It breaks the Liskov Substitution Principle
2. Because of it it is impossible to use SiweMessage in FastAPI or other BaseModels

That just solves 2, and it's better to have the simpler constructor and having a separate classmethod for strings

[sbihel]

Sorry about that, I opened #61