spruceid / siwe-py

A Python implementation of Sign-In with Ethereum
https://login.xyz
Apache License 2.0
66 stars 28 forks source link

AttributeError: 'SiweMessage' object has no attribute 'chain_id' #14

Closed twinsant closed 2 years ago

twinsant commented 2 years ago
  File "/Users/ant/GitHub/ethos/src/ethos/main.py", line 48, in post
    siwe_message.validate()
  File "/Users/ant/miniconda3/envs/ws/lib/python3.9/site-packages/siwe/siwe.py", line 188, in validate
    message = eth_account.messages.encode_defunct(text=self.sign_message())
  File "/Users/ant/miniconda3/envs/ws/lib/python3.9/site-packages/siwe/siwe.py", line 175, in sign_message
    message = self.to_message()
  File "/Users/ant/miniconda3/envs/ws/lib/python3.9/site-packages/siwe/siwe.py", line 130, in to_message
    chain_field = f"Chain ID: {self.chain_id or 1}"
AttributeError: 'SiweMessage' object has no attribute 'chain_id'

The message format just copy from notepad example.

while the message content is like this:

    {'domain': '127.0.0.1:4003',
     'address': '0x464eE0FF90B7aC76d3ec8D2a25E6926DeCC88f6d',
     'chainId': '1',
     'uri': 'http://127.0.0.1:4003',
     'version': '1',
     'statement': 'EthOS',
     'type': 'Personal signature',
     'nonce': 'some nonce',
     'issuedAt': '2022-01-24T02:32:10.239Z',
     'signature': 'some signature'
     }
    '''

My question is the field name seems to be not be right, according EIP-4361, the correct field name should has a '-' in it .

So, need I convert the chainId into chain-id ? or we should fix the python lib as well?

payton commented 2 years ago

Hey @twinsant,

Thanks for the question and raising this issue!

It looks like you are referencing the "Informal Message Template" section of EIP-4361. This defines the given template as being A Bash-like informal template of the full message. The naming of the variables is an example of how the standard can be implemented. The naming convention of the variables themselves is out of the scope of the standard.

Naming in this repository follows PEP 8 naming conventions which can be found here: https://www.python.org/dev/peps/pep-0008/#function-and-variable-names

TLDR is that Python should follow snake case naming conventions for class variables. In the context of this issue, chainId should be converted to chain_id. When I first built this implementation, I converted camel case to snake case, but I now think that should be left out of the library itself.

I'll leave it to @sbihel for the official conclusion on this issue being a representative of Spruce.

sbihel commented 2 years ago

Hiya,

Payton is right, interoperability is only expected with the serialised message form. You can pass around JSON if you control things on both ends but it's not recommend. If you do chose to go down this road you can use something like the decamelize library and do decamelize(json.load(fp=my_json_message)) to convert the casing automatically.