folia-app / folia-contracts

Apache License 2.0
19 stars 7 forks source link

Proposal: remove exists flag to make contract cheaper #5

Closed vrde closed 1 year ago

vrde commented 3 years ago

My assumption is that a work with no editions doesn't exist. In this context, Work.exists can be removed and replace by checking for Work.editions > 0.

Another approach would have been to check if Work.address is defined, but it's cheaper (at deploy time) to check if editions is > 0.

I've also added a require in addArtwork to check if the artist's address is set.

okwme commented 3 years ago

i'm a little wary of this update as you can see it removes the ability to create a work with an edition of 0. This is actually a flow that I think we should preserve as works increment the work ID. In case we may want to reserve a work ID for something that hasn't been released yet we could create it as an edition of 0 and then update it at a later point (we could of course also make it an edition of 1 and then update it at a later point but this could cause confusion about the actual number of editions).

in general i'm in favor of less gas efficient but more expressive, however the check for an artist address is a good idea.

vrde commented 3 years ago

@okwme sure, wasn't aware of this use case, it makes sense. I left this PR in draft so we can experiment :)

in general i'm in favor of less gas efficient but more expressive, however the check for an artist address is a good idea.

Do you think that checking for Work.address instead of Work.exists is a good idea?

Alternatively, given that a bool is a uint8, we can merge the two bools exists and paused in a generic state bitmap.

struct Work {
        // 00000000
        //        ^-- exists
        //       ^-- paused
        uint8 state;
        uint256 editions;
        uint256 printed;
        uint256 price;
        address payable artist;
}

Anyway, you point about readability is important and I don't want to be pedantic, if there is any idea here that you think is worth it I can put it in this PR :)