core-contracts icon indicating copy to clipboard operation
core-contracts copied to clipboard

feat: implement state replace contract

Open austinabell opened this issue 3 years ago • 8 comments

closes #203

cc @esaminu does this fit the criteria for what wallet needs?

What has changed:

  • interface changed from '{"keys":["...", "..."]}' to '["...", "..."]' to avoid redundant strings.
    • This won't work with near-cli because of this https://github.com/near/near-cli/issues/920 so I'm happy to add the redundant data
  • Changed to no_std contract -- size down from 96k -> 67k (70k with new functionality)
    • Extra boilerplate, would be nice to use https://github.com/austinabell/nesdie to avoid, but wanted to keep minimal dependencies for security
    • Can switch to using SDK when a middle layer for no_std is implemented
  • Optimized deserialization -- ~~no longer allocates or copies values in any way~~ Edit: it does now because the stream deserialization was not correct, I'll look into it later
    • Previously allocated every single encoded key string as well as a Vec buffer of all strings, now none of that -- optimized for gas usage

Did not change the serialization from serde because it was indicated JSON is preferred. Can switch to using https://github.com/dtolnay/miniserde for very small code size, but it will come at the cost of less efficient deserializations. If gas optimizations are important, can open an issue to swap serialization by a feature and compare the differences.

austinabell avatar Mar 31 '22 21:03 austinabell

Looks good! We'd have to update how we call it in near-api-js due to the interface change but aside from that we should be good to go. For the state restoration operation suggested here we can just use one transaction with function calls to first clean and then storage_write to add the new state in near/near-wallet#2561.

cc: @MaximusHaximus

esaminu avatar Apr 01 '22 17:04 esaminu

Looks good! We'd have to update how we call it in near-api-js due to the interface change but aside from that we should be good to go.

Very quick and easy to just have the format match the previous one if you'd like, but felt uncomfortable adding the redundancy in the serialization format personally.

For the state restoration operation suggested here we can just use one transaction with function calls to first clean and then storage_write to add the new state in near/near-wallet#2561.

Yeah, so it's more optimized if you just deploy one contract to do both to avoid pushing duplicate code. All three combinations are provided with this (cleanup, replace, both) but they are 67k, 69k, and 70k bytes respectively so very negligible.

austinabell avatar Apr 01 '22 18:04 austinabell

~~hmm, hold up on merging this though, adding workspaces tests seemed to brick the build -- diving into why that affected it~~ fixed

austinabell avatar Apr 01 '22 18:04 austinabell

Awesome! Can this be reviewed and merged? @MaximusHaximus @agileurbanite

esaminu avatar Apr 01 '22 22:04 esaminu

@austinabell after further discussion with the team, we think it would be better to leave the clean method signature as it currently is due to serialization issues with array args in near-api-js. Aside from that we'd love to see this merged!

esaminu avatar Apr 07 '22 20:04 esaminu

@austinabell after further discussion with the team, we think it would be better to leave the clean method signature as it currently is due to serialization issues with array args in near-api-js. Aside from that we'd love to see this merged!

Done :) Sorry for being opinionated and trying to avoid tech debt for this.

format is now {"keys":...} for clean and {"entries: [[.., ..], ...]} for replace, as readme shows.

Also updated workspaces and the tests now that that's released so this is good timing :D

austinabell avatar Apr 08 '22 00:04 austinabell

Awesome! Would love to see this merged :)

esaminu avatar Apr 08 '22 17:04 esaminu

Is there anything blocking this? Who is reviewing/approving this change?

austinabell avatar May 18 '22 17:05 austinabell