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

Addressable ids #19

Merged
merged 5 commits into from
Jun 28, 2022
Merged

Addressable ids #19

merged 5 commits into from
Jun 28, 2022

Conversation

topocount
Copy link
Contributor

@topocount topocount commented Jun 14, 2022

This PR creates hat ids that indicate a hat's relationship to other hats.
The address can be parsed to find admins, and determine the hat's location
in a hat hierarchy without accessing storage. Child hats are now accessible
directly from an admin as well, though that requires peeking at storage.

This comes at the tradeoff of limiting a hierarchy (tree) to a depth of six
hat levels. and each hat may only administer a total of 255 hats directly.

I chose a depth of six because it seemed like a future-proof quantity of
levels for larger orgs, but we could change it to 4 and be back to a uint64.

Note, this code is still non-optimized so there are still improvements to be made in most bitwise operations. I tried to keep it in readable Solidity for the proof of concept.

src/Hats.sol Outdated
* for 255 different hats.
*
*/
mapping(uint80 => Hat) public hats;

mapping(uint64 => uint32) public hatSupply; // key: hatId => value: supply
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mapping(uint80 => uint32) public hatSupply; // key: hatId => value: supply

src/Hats.sol Show resolved Hide resolved
@@ -380,37 +394,28 @@ contract Hats is ERC1155 {
/// @param _conditions The address that can deactivate the hat
/// @return hatId The id of the newly created hat
function _createHat(
Copy link
Member

Choose a reason for hiding this comment

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

I think this function also needs to increment hat.lastHatId

@@ -716,7 +728,7 @@ contract Hats is ERC1155 {
uint256 amount,
bytes memory data
) internal override {
_balanceOf[to][id] += amount;
//_balanceOf[to][id] += amount;
Copy link
Member

Choose a reason for hiding this comment

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

how come you're removing this line? If I'm interpreting correctly, without this line _mint would not change any 1155 token balances. What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it just wasn't valid solidity so i shelved it for now

Copy link
Member

@spengrah spengrah Jun 15, 2022

Choose a reason for hiding this comment

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

ah I think I know what happened. I changed the ERC1155._balanceOf function from private to internal to enable Hats.sol to call it, but hadn't pushed that change up yet. I'm a bit confused how to handle the situation since I sort of forked the solmate repo. May need to bring that forked repo into the Hats Protocol org...

I just up the change, here. You can probably rebase and the code should now work. Let me know if it doesn't.

@@ -734,7 +746,7 @@ contract Hats is ERC1155 {
uint256 id,
uint256 amount
) internal override {
_balanceOf[from][id] -= amount;
//_balanceOf[from][id] -= amount;
Copy link
Member

Choose a reason for hiding this comment

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

same question here as for _mint

@spengrah
Copy link
Member

I chose a depth of six because it seemed like a future-proof quantity of
levels for larger orgs, but we could change it to 4 and be back to a uint64.

What would be the downside of ramping all the way up from uint80 to uint256 for hatIds? That would allow us to support ( 256 - 32 ) / 8 = 28 levels. And as a smaller benefit would allow us to avoid having to type cast when working with ERC1155 standard functions.

Since we're no longer storing hatIds in the Hat struct, my anticipation is that doing so wouldn't increase any storage costs.

@spengrah
Copy link
Member

In general, this is amazing! Well done @topocount 🙌

I left a few questions and comments above. Thanks!

@topocount
Copy link
Contributor Author

I chose a depth of six because it seemed like a future-proof quantity of
levels for larger orgs, but we could change it to 4 and be back to a uint64.

What would be the downside of ramping all the way up from uint80 to uint256 for hatIds? That would allow us to support ( 256 - 32 ) / 8 = 28 levels. And as a smaller benefit would allow us to avoid having to type cast when working with ERC1155 standard functions.

Since we're no longer storing hatIds in the Hat struct, my anticipation is that doing so wouldn't increase any storage costs.

This is totally doable. if you think it'd a good product move i'll do it. Was just trying to see how small i could make everthing while allowing for plenty of breathing room.

@spengrah
Copy link
Member

I chose a depth of six because it seemed like a future-proof quantity of
levels for larger orgs, but we could change it to 4 and be back to a uint64.

What would be the downside of ramping all the way up from uint80 to uint256 for hatIds? That would allow us to support ( 256 - 32 ) / 8 = 28 levels. And as a smaller benefit would allow us to avoid having to type cast when working with ERC1155 standard functions.

Since we're no longer storing hatIds in the Hat struct, my anticipation is that doing so wouldn't increase any storage costs.

This is totally doable. if you think it'd a good product move i'll do it. Was just trying to see how small i could make everthing while allowing for plenty of breathing room.

Ok great! If there are no (or minimal) storage cost downsides, support for as many levels as possible is ideal

@spengrah
Copy link
Member

This address #18

@spengrah spengrah merged commit 7fc6ca5 into Hats-Protocol:mvp Jun 28, 2022
@nintynick nintynick mentioned this pull request Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants