Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement state replace contract #208

Merged
merged 12 commits into from Jun 30, 2023
Merged

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Mar 31, 2022

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.
  • 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.

@esaminu
Copy link

esaminu commented Apr 1, 2022

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

@austinabell
Copy link
Contributor Author

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
Copy link
Contributor Author

austinabell commented Apr 1, 2022

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

@esaminu
Copy link

esaminu commented Apr 1, 2022

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

@esaminu
Copy link

esaminu commented Apr 7, 2022

@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!

@austinabell
Copy link
Contributor Author

@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

@esaminu
Copy link

esaminu commented Apr 8, 2022

Awesome! Would love to see this merged :)

@austinabell
Copy link
Contributor Author

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

@frol frol merged commit 3f3170f into master Jun 30, 2023
@bucanero bucanero deleted the austin/state_manipulation branch September 14, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize state clearing contract
3 participants