-
Notifications
You must be signed in to change notification settings - Fork 51
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
feat: add SRC-16 Typed Structured Data #161
base: master
Are you sure you want to change the base?
feat: add SRC-16 Typed Structured Data #161
Conversation
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Antony <b***@g***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Fuel Labs Contributor License Agreement and this Pull Request will be revalidated. |
Thanks for the contribution! Before we can merge this, we need @CatspersCoffee to sign the Fuel Labs Contributor License Agreement. |
Just giving you a heads up, we will be reviewing this standard in the following days, but Fuel is enacting a code freeze around christmas as many contributors will be on vacation etc. So this will likely not be merged until January as the earliest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing!
Just a few questions, primarily surrounding whether the existing encoder and hashing functionality in the std-lib and core are sufficent/can be integrated or if we need to introduce new traits and structs.
I'm also curious on how handling Fuel specific types would work. We've made the distinct choice to separate the Address
and ContractId
types as well as have AssetId
.
|
||
Atomic Types: | ||
```sway | ||
bytes1 to bytes32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would this look like in practice? Sway does not have a bytes1
type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankyou for pointing out this mistake. This has been corrected to:
Atomic Types:
u8 to u256
bool
b256 (hash)
Dynamic Types:
Bytes // Variable-length byte sequences
String // Variable-length strings
Reference Types:
Arrays (both fixed size and dynamic)
Structs (reference to other struct types)
/// A demo struct representing a mail message | ||
pub struct Mail { | ||
/// The sender's address | ||
pub from: b256, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub from: b256, | |
pub from: Address, |
pub from: b256, | |
pub from: ContractId, |
pub from: b256, | |
pub from: Identity, |
To follow Sway principles, this should use either Address
, ContractId
, or Identity
.
The same applies to to
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been adjusted in the Fuel example to reflect the Address
native type.
/// A demo struct representing a mail message
pub struct Mail {
/// The sender's address
pub from: Address,
/// The recipient's address
pub to: Address,
/// The message contents
pub contents: String,
}
standards/src/src16.sw
Outdated
/// * [b256] - The Keccak256 hash of the encoded domain parameters | ||
/// | ||
pub fn domain_hash(self) -> b256 { | ||
let mut encoded = Bytes::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it could use the AbiEncode
trait from core https://fuellabs.github.io/sway/master/core/codec/index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankyou for the suggestion. I have added:
impl AbiEncode for SRC16Domain { ... }
and
impl AbiEncode for EIP712Domain { ... }
|
||
/// Trait for types that can be hashed in a structured way | ||
/// | ||
pub trait TypedDataHash { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a new trait or will Hash
work here? I believe the implementations are the same. https://fuellabs.github.io/sway/master/std/hash/trait.Hash.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This trait is required as the implementer needs to write (or generate) TypedDataHash
for <Their_Struct_Type>
in line with the definition of hashStruct
.
The TypedDataHash
is not a simple Hasher, but an encoding scheme and final hash.
…SRC16Domain fuel_example contract, Adjust types for domains. Update docs.
Type of change
Changes
The following changes have been made:
SRC16Domain
(Fuel) andEIP712Domain
(Ethereum).Notes
Related Issues
Closes #
Checklist
New Feature
labels where relevant.