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

Ensure generic values used with a StorageMap are hashable #5045

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

bitzoic
Copy link
Member

@bitzoic bitzoic commented Aug 28, 2023

Description

Currently, any type may be included as a key value to the StorageMap<K, V> type. This presents an issue as the type must be hashable in order to generate the key.

A where clause has been added to the function definition.

This should open the door for using heap types(String, Bytes) as key values for the StorageMap type however, this will require additional compiler changes.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@bitzoic bitzoic added the lib: std Standard library label Aug 28, 2023
@bitzoic bitzoic requested a review from a team August 28, 2023 12:27
@bitzoic bitzoic self-assigned this Aug 28, 2023
@bitzoic bitzoic linked an issue Aug 28, 2023 that may be closed by this pull request
@bitzoic bitzoic requested a review from IGI-111 August 28, 2023 12:33
Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add breaking label?

I don't really understand the security issue. I see it more as a potential compiler issue.
Don't really see an issue with this constraint though.

@bitzoic
Copy link
Member Author

bitzoic commented Aug 28, 2023

Add breaking label?

I don't really understand the security issue. I see it more as a potential compiler issue. Don't really see an issue with this constraint though.

Perhaps not security concern, this has been corrected. But we still see the following:

This compiles:

struct MyStruct {
    val: u64
}

abi MyContract {
    #[storage(read, write)]
    fn test_function();
}

storage {
    my_map: StorageMap<MyStruct, u64> = StorageMap {},
}

impl MyContract for Contract {
    #[storage(read, write)]
    fn test_function() {
        let my_struct = MyStruct { val: 1 };
        storage.my_map.insert(my_struct, 2)
    }
}

When this does not, even though the insert() function uses the same sha256<T>() function:

use std::hash::sha256;

struct MyStruct {
    val: u64
}

abi MyContract {
    fn test_function();
}

impl MyContract for Contract {
    fn test_function() {
        let my_struct = MyStruct { val: 1 };
        let result = sha256(my_struct);
    }
}

@bitzoic bitzoic added the breaking May cause existing user code to break. Requires a minor or major release. label Aug 28, 2023
@bitzoic bitzoic requested a review from a team August 28, 2023 13:29
@Braqzen
Copy link
Contributor

Braqzen commented Aug 28, 2023

Add breaking label?
I don't really understand the security issue. I see it more as a potential compiler issue. Don't really see an issue with this constraint though.

Perhaps not security concern, this has been corrected. But we still see the following:

This compiles:

struct MyStruct {
    val: u64
}

abi MyContract {
    #[storage(read, write)]
    fn test_function();
}

storage {
    my_map: StorageMap<MyStruct, u64> = StorageMap {},
}

impl MyContract for Contract {
    #[storage(read, write)]
    fn test_function() {
        let my_struct = MyStruct { val: 1 };
        storage.my_map.insert(my_struct, 2)
    }
}

When this does not, even though the insert() function uses the same sha256<T>() function:

use std::hash::sha256;

struct MyStruct {
    val: u64
}

abi MyContract {
    fn test_function();
}

impl MyContract for Contract {
    fn test_function() {
        let my_struct = MyStruct { val: 1 };
        let result = sha256(my_struct);
    }
}

hm, strange. seems like a compiler issue.
the struct is a compile time construct so it doesn't really exist. all the values are tightly packed so the hashing should work. it seems like the call opcode can take a struct and deref into the value.
idk, something is broken though

Copy link
Contributor

@esdrubal esdrubal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue that reports the faulty behavior found in this PR. #5046

Although the added constraints are redundant because the impl self already contains them, because the compiler appears not to check the impl self constraints I think this is a good idea to add until the issue is fixed.

@bitzoic bitzoic merged commit 218f0da into master Aug 28, 2023
@bitzoic bitzoic deleted the bitzoic-5044 branch August 28, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking May cause existing user code to break. Requires a minor or major release. lib: std Standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants