feat: add pushToField to Store and World #434
Conversation
| @@ -48,7 +48,6 @@ library AddressArray_ { | |||
|
|
|||
| /** | |||
| * Push an element to the addresses array | |||
| * TODO: this is super inefficient right now, need to add support for pushing to arrays to the store core library to avoid reading/writing the entire array | |||
There was a problem hiding this comment.
Curious to see if we have gas tests for before/after this change to see how much more efficient this method is compared to the previous.
There was a problem hiding this comment.
The real change would be when a separate event is added. Right now there's still a redundant full storage read
There was a problem hiding this comment.
there should be a gasreport for these changes too though (there're some for push too), wonder why it didn't get added
|
we need to rebase this one on main now |
| * and return `tableRouteId` for `accessRoute/subRoute` | ||
| * because access to a route also grants access to all sub routes. | ||
| */ | ||
| function _verifiedTableRouteId(string calldata accessRoute, string calldata subRoute) |
There was a problem hiding this comment.
might make sense to use this internal function for other methods in here too. Would leave that for a follow up though, thinking about slightly changing the accessRoute/subRoute pattern
There was a problem hiding this comment.
yep I made it to remove code repetition but didn't wanna bloat this PR
| @@ -83,6 +81,6 @@ library AddressArraySchemaLib { | |||
|
|
|||
| function decode(bytes memory blob) internal pure returns (address[] memory addresses) { | |||
| if (blob.length == 0) return new address[](0); | |||
| SliceLib.getSubslice(blob, 32).decodeArray_address(); | |||
| return SliceLib.getSubslice(blob, 32).decodeArray_address(); | |||
There was a problem hiding this comment.
Good catch - is this schema used anywhere? Weird that this wasn't caught by any tests..
There was a problem hiding this comment.
I think it broke something for me, which is how I noticed it. Don't remember what though, could have even been some temporary test I used AddressArray for
packages/world/test/World.t.sol
Outdated
| vm.expectRevert(abi.encodeWithSelector(World.RouteAccessDenied.selector, route, caller)); | ||
| } | ||
|
|
||
| function _initRouteTableKey( |
There was a problem hiding this comment.
This is a personal preference, but I'm not the biggest fan of helper functions in tests. They reduce verbosity, but in tests I prefer to see everything that happens explicitly without having to check the logic of a helper function. And if helper functions in tests become more complex, we would need to add tests for the helper functions too haha.
This helper function is simple enough to keep it if you feel strongly about it though.
There was a problem hiding this comment.
agree in general, and for _initRouteTableKey specifically, see #434 (comment)
for _expectRouteAccessDenied I think it's an exception. It's used a lot (12 times) with the same pattern. caller is used twice there, so without an internal method higher risk of making a mistake. Also changed it to prank+expectRevert (it was even longer with startPrank+expectRevert+stopPrank, which is yet another mistake risk).
So I refactored it more, to use _expectRouteAccessDenied in all 12 cases
|
Looks like we need to resolve the gas-report conflicts with main (easiest way would be to merge main into this and just rerun the gas report) |
alvrs
changed the title
Intentionally not adding a new event, but the method's logic accounts for it being added in the future with minimal changes to StoreCore and no changes to World etc (so it could be mostly a network PR)
Notably this allows pushing multiple items at once
No validity checks though (same as setField, they should be added in one separate refactor I think)
_verifiedTableRouteId,_expectRouteAccessDeniedand_initRouteTableKey:I basically want to reduce code repetition in World but I don't want to complicate this PR by refactoring things unrelated to
pushToStoreyetsetItem and pop could happen in the future, but rn I just need push for proper autogen