Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SNIP12: Incorrect use of u64 in docs example #1273

Closed
trishtzy opened this issue Dec 24, 2024 · 1 comment · Fixed by #1274
Closed

SNIP12: Incorrect use of u64 in docs example #1273

trishtzy opened this issue Dec 24, 2024 · 1 comment · Fixed by #1274

Comments

@trishtzy
Copy link
Contributor

trishtzy commented Dec 24, 2024

📝 Details

The example in SNIP12 incorrectly uses u64 for the expiry field, which is not a supported data type from the official SNIP12 specification.

struct Message {
    recipient: ContractAddress,
    amount: u256,
    nonce: felt252,
    expiry: u64 // not supported, should be u128
}

https://github.com/OpenZeppelin/cairo-contracts/blob/v0.20.0/docs/modules/ROOT/pages/guides/snip12.adoc?plain=1#L76

When trying to re-create the Message struct for off-chain signing using Golang or Python, it results in compilation errors when hashing the Typed Message.

🔢 Code to reproduce bug

package main

import (
  "encoding/json"
  "fmt"

  "github.com/NethermindEth/starknet.go/typedData"
)

func main() {
  jsonData := `{
    "types": {
      "StarknetDomain": [
        { "name": "name", "type": "shortstring" },
        { "name": "version", "type": "shortstring" },
        { "name": "chainId", "type": "shortstring" },
        { "name": "revision", "type": "shortstring" }
      ],
      "Message": [
        { "name": "recipient", "type": "ContractAddress" },
        { "name": "amount", "type": "u256" },
        { "name": "nonce", "type": "felt" },
        { "name": "expiry", "type": "u64" }
      ]
    },
    "primaryType": "Message",
    "domain": {
      "name": "StarkNet Mail",
      "version": "1",
      "chainId": "1",
      "revision": "1"
    },
    "message": {
      "recipient": "0xd392b0c0500700d02d27ab30805ec80ddd3320ff",
      "amount": "100.00",
      "nonce": 0,
      "expiry": 1734859800
    }
  }`

  var ttd typedData.TypedData
  err := json.Unmarshal([]byte(jsonData), &ttd)
  if err != nil {
    panic(fmt.Errorf("fail to unmarshal TypedData: %w", err)) // `panic: fail to unmarshal TypedData: can't parse type u64`
  }
  messageTypeHash, err := ttd.GetTypeHash("Message")
  if err != nil {
    panic(fmt.Errorf("fail to get message type hash: %w", err))
  }
  fmt.Println("message type hash:", messageTypeHash)
}

Running the Go script results in panic: fail to unmarshal TypedData: can't parse type u64

Could I open a pull request to update the example?

const MESSAGE_TYPE_HASH: felt252 = 0xa2a7036c1f406af7c47722b209f23bd2f2d6ac21423c8c73bd92cf28409ee2; // updated hash

#[derive(Copy, Drop, Hash)]
struct Message {
    recipient: ContractAddress,
    amount: u256,
    nonce: felt252,
    expiry: u128 // updated 
}
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Contracts for Cairo Dec 24, 2024
@trishtzy trishtzy changed the title SNIP12: SNIP12: Incorrect use of u64 in document example Dec 24, 2024
@trishtzy trishtzy changed the title SNIP12: Incorrect use of u64 in document example SNIP12: Incorrect use of u64 in docs example Dec 24, 2024
@andrew-fleming
Copy link
Collaborator

Thanks, @trishtzy! Yeah, feel free to open a PR fixing this. Note though that we keep the actual struct member as type u64. We only use u128 for encoding to be compliant with snip12, so really just the type hash and its computation need to be updated.

It's probably helpful to leave a comment regarding u64 <> u128 in the guide:

// Since there's no u64 type in SNIP-12, we use u128 for `expiry` in the type hash generation.
let message_type_hash = ...

andrew-fleming pushed a commit that referenced this issue Jan 2, 2025
* docs: fix message type hash in snip12 guide

* changelog: update
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Resolved in Contracts for Cairo Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Resolved
Development

Successfully merging a pull request may close this issue.

2 participants