hyperlane-xyz / fuel-contracts

5 stars 2 forks source link

Ownership library, make Mailbox owned #31

Closed tkporter closed 1 year ago

tkporter commented 1 year ago

There are a few constraints with Sway that lead to this pretty ugly implementation:

  1. There are no constructors to set storage in. Instead, you must hardcode initial storage values in the storage {} block
  2. Storage support in libraries is super weak. You can get around this for now by doing something like what StorageMerkleTree / StorageVec / StorageMap does - which involves using the intrinsic __get_storage_key() to get the storage key of the struct, and then using store and get as an API to write and read directly to / from storage. This works well for stored data structures that don't require special initialization. When it comes to initializing with specific, things can get a little messy. It's possible to have:
    
    // in the storage block

owner: StorageOwner { owner: Option::Some(Identity::Address(Address::from(0xcafecafecafecafecafecafecafecafecafecafecafecafecafecafecafecafe))), }


But then when it comes to accessing this state manually using `store` and `get`, which require the raw b256 storage key, you need to worry about how storage key locations are generated for structs / enums. They storage keys are generated can be found in [this Discord msg](https://discord.com/channels/732892373507375164/1034937683581083679/1046826502127362048), or also in this function [`get_storage_key`](https://github.com/FuelLabs/sway/blob/master/sway-core/src/ir_generation/storage.rs#L19). Basically if that `owner: StorageOwner` is at index 1 in the storage block, then the `Option<Identity>` parts ends up being stored at really specific locations:

// from the hyperlane-mailbox-storage_slots.json file [ // I believe this is the Option part. Option::Some is shown with the 0000000000000001, otherwise it'd be all zeroes to indicate None. // This storage key is the result of sha256("storage_1_0_0") (u can try this yourself too) { "key": "6015238849053244ee5389f353661a2997d73545a6b7ee1a91cbfaad71eafebf", "value": "0000000000000001000000000000000000000000000000000000000000000000" }, // This is the Identity part. The zero prefix indicates it's the Address variant. Then afterward comes the variant contents, // which is the Address. This storage key is the result of sha256("storage_1_0_1") { "key": "42f8651cb9da7da6f1c35e8a98476cae169c675cc49bab9f454177faa48ac9d9", "value": "0000000000000000cafecafecafecafecafecafecafecafecafecafecafecafe" }, { "key": "42f8651cb9da7da6f1c35e8a98476cae169c675cc49bab9f454177faa48ac9da", "value": "cafecafecafecafe000000000000000000000000000000000000000000000000" }, ...



So basically figuring out the exact storage slot to be reading if we want to initialize storage slots and also manipulate them in the library is totally untenable. This also feels crazy fragile to try to replicate some of that `sha256("storage_1_0_1")` garbage in the library

4. You can't inherit from another contract in a contract. Meaning you must implement abis for your contract yourself within your contract. The best you can get is to have a common abi that you implement yourself

So this left a few options:
1. Create a `StorageOwner`, go with the "manual" storage management, and don't have any initial storage set. We'd store something like `{ initialized: bool, owner: Option<Identity> }`. This felt bad for a couple reasons:
  1. Not setting the owner initially would mean that the first person to call an `initialize` function would be able to claim ownership. Shifting the burden of doing this to the deployer and requiring them to be aware that someone theoretically could frontrun them and claim ownership feels concerning. Imo not having to think about this whatsoever is much preferred
  2. We'd put in a lot of effort for an implementation like this that may end up being obsolete if the Fuel team does make a big rework of how storage works with libraries. Generally I think we should try to avoid a bunch of time on really bespoke library setups if we can
2. Don't even have a shared ownership management library, and do everything inside the Mailbox contract. This feels dumb because we'll need ownership for some other contracts.
3. What's described below

What's included in this PR:

There's a library created called `ownership`. This doesn't have any actual management of ownership, but has some tools to help consumers manage ownership.

In the Mailbox's storage block, the `owner: Option<Identity>` is declared directly.

In the Mailbox, we then implement the `Ownable` abi that is originally declared in the `ownership` library. This has 2 functions - `owner() -> Option<Identity>`, which gets the owner, and `transfer_ownership(new_owner: Option<Identity>)`, which transfers ownership.

Back in the ownership library, we have 2 helper functions:
* `require_msg_sender(owner: Option<Identity>)`, which requires the msg_sender to be the provided owner.
* `log_ownership_transferred`, which will log a special struct `OwnershipTransferredEvent` that indicates that ownerhsip was transferred

Happy to chat more about why I think this is the best option in the short term.

Longer term, assuming that storage library support gets better, we should def move to a `StorageOwner` type struct. But for now I think this is sufficient as long as we test sufficiently in ownable contracts.