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

State reloading pattern for receiver hooks #139

Open
alexytsu opened this issue Oct 24, 2022 · 2 comments
Open

State reloading pattern for receiver hooks #139

alexytsu opened this issue Oct 24, 2022 · 2 comments

Comments

@alexytsu
Copy link
Collaborator

alexytsu commented Oct 24, 2022

I don't know if it's a good idea for the library code to be doing this instead of handling state loading on the user implementation side (like we're doing for fungible tokens). It'll be ok for now since it's going into a dev branch but I think it'll cause problems in actor code that carries additional state of its own beyond the NFTState

It does open up one easy optimisation though: we can pass through the original MintReturn data in the intermediate struct and simply return that if there's no change once the hook call completes. I've been playing with the idea here (sending original data) and here (trying to centralise the 'has state changed' check) for the fungible token

Originally posted by @abright in #138 (comment)

@anorth
Copy link
Member

anorth commented Feb 2, 2023

Possibly related to #110

@abright
Copy link
Contributor

abright commented Feb 14, 2023

It's definitely related to #110 but I've closed that one so this can be the sole "hook return optimisation" issue

My two commits linked in the original comment here played with adding the original (pre-hook) mint and transfer return structs in as part of the intermediate structs we carry through the hook code. This side of it is an easy optimisation as it isn't carrying much more data but can avoid all the extra reads after the hook completes, in the common case of the state being unchanged (where a hook just accepts and takes no further action). The tricky part is finding a clean and ergonomic way to handle checking for changes to the state before/after the hook call and passing that result along to the hook return functions (eg: transfer_return()).

I don't want to have the check/reload logic happening inside the existing hook-related functions - it feels too 'magical' and will probably conflict with more complex token implementations like frc46_factory_token due to the way they handle state. The NFT library does this but the fungible token leaves this up to the user implementation to figure out. I played around with passing in a bool to the return functions to signal a state change but that felt way too easy to mess up. Checking the CID itself could be done around the hook call (with CID or a flag of some sort saved in the intermediate struct) but either the function making the call or maybe the return function afterwards would need a way to reload so it had up-to-date state - probably by passing in a function/closure so it's easy to adapt to a given token implementation.

I think it was on the right track but needed some more thought put into the design, which should be a bit easier when there's some more token implementations (with additional state of their own beyond the TokenState) to consider.

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

No branches or pull requests

3 participants