latticexyz / mud

MUD is a framework for building autonomous worlds
https://mud.dev
MIT License
757 stars 189 forks source link

`.set` in single value tables should emit SetRecord not Splice event #479

Open holic opened 1 year ago

holic commented 1 year ago

Currently single value tables autogenerate their .set(value) call to use store.setField. This broke an assumption I had in https://github.com/latticexyz/mud/pull/415/files#diff-a6b694ab744d9808772b7fb1cfc0a286ee20f371069cf905c4e1af8b70bf5d68

I can work around this in the client to check if a StoreSetField event actually has all the data defined by the schema (and thus is equivalent to a StoreSetRecord), but I am wondering if this change would be more "correct" by doing it at the autogen level.

dk1a commented 1 year ago

Currently single value tables autogenerate their .set(value) call to use store.setField

I did this because setField is easier to generate and cheaper gas-wise (probably, likely not by much). It's not particularly difficult to change. Single value tables with a dynamic value would still keep their push (and pop/setItem when those get added), which could still break the StoreSetRecord assumption, so imo it's best to not have that assumption on the network side. For multi value tables users could also opt to never use the full set and just set individual fields, which also breaks that assumption

alvrs commented 1 year ago

TODO: evaluate gas impact. If significant fine to keep as is imo.

holic commented 1 year ago

We should reevaluate this in terms of gas, because the perks of using SetRecord over SpliceData is: