bosonprotocol / boson-protocol-contracts

Boson Protocol V2 (latest)
https://bosonprotocol.io/
GNU General Public License v3.0
32 stars 8 forks source link

Diamond Storage Comments #249

Closed mudgen closed 2 years ago

mudgen commented 2 years ago

The contracts/Solidity code is using Diamond Storage correctly.

There are a few structs being used directly in other structs that are stored in contract storage. Inner structs cannot be extended in future upgrades. This is okay if it is known that they will never need to be extended in the future.

For example the Voucher struct here: https://github.com/bosonprotocol/boson-protocol-contracts/blob/9639040566d9a6b355355009a2a6d5ef7be9327e/contracts/domain/BosonTypes.sol#L158

Is used directly in the Exchange struct here: https://github.com/bosonprotocol/boson-protocol-contracts/blob/9639040566d9a6b355355009a2a6d5ef7be9327e/contracts/domain/BosonTypes.sol#L149

If desired there is a technique that can be used to make inner structs extendable in future upgrades. The technique is to put the inner struct in a mapping within the outer struct.

For example see voucher in this code:

struct Exchange {
    uint256 id;
    uint256 offerId;
    uint256 buyerId;
    uint256 finalizedDate;
    Voucher voucher;
    ExchangeState state;
}

struct Voucher {
    uint256 committedDate;
    uint256 validUntilDate;
    uint256 redeemedDate;
    bool expired;
}

And here is the change:

struct Exchange {
    uint256 id;
    uint256 offerId;
    uint256 buyerId;
    uint256 finalizedDate;
    mapping(uint256 => Voucher) voucher;
    ExchangeState state;
}

struct Voucher {
    uint256 committedDate;
    uint256 validUntilDate;
    uint256 redeemedDate;
    bool expired;
}

A constant can be defined that is used to access any struct that uses this trick:

string constant INNER_STRUCT= 0;

Exchange storage exchange = ProtocolLib.protocolEntities().exchanges[5];

Voucher storage voucher = exchange.voucher[INNER_STRUCT]
cliffhall commented 2 years ago

Great tip, @mudgen. Will review the storage for any of these cases we need to clear up.

zajck commented 2 years ago

Fixed with #268 and #272