5afe / safe-core-protocol-specs

Safe{Core} Protocol is an open, modular framework to make smart accounts secure, portable, and composable.
GNU General Public License v3.0
67 stars 13 forks source link

[Protocol Specs] "SafeTransaction and SafeRootAccess MUST have an id requirement" is vague #32

Open mmv08 opened 1 year ago

mmv08 commented 1 year ago

The manager specification states:

Both SafeTransaction and SafeRootAccess MUST have a unique ID, which is the EIP-712 hash of the object.

https://github.com/safe-global/safe-core-protocol-specs/blob/681c63678bfdd0a0087bbc26702979df769c50da/manager/README.md

This particular requirement seems a tad unclear to me because it can be understood in multiple ways:

  1. SafeTransaction and SafeRootAccess must have an ID field
  2. It should be possible to compute a unique identifier based on struct fields

The real meaning seems to be 2), judging by the Uniqueness section below. In my opinion, the current specs do not clearly define how to calculate an identifier (at least a basic example) and leave it slightly ambiguous. The general types section suggests using an EIP-712 hash, and the Uniqueness section requires it to be unique without uniqueness requirements (e.g., it should be uniquely identifiable across chains).

My suggestion: