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

generate return value from current state #138

Merged
merged 1 commit into from
Oct 24, 2022
Merged

generate return value from current state #138

merged 1 commit into from
Oct 24, 2022

Conversation

alexytsu
Copy link
Member

Finishes up receiver hooks for transfer and transfer_from

@alexytsu alexytsu marked this pull request as ready for review October 21, 2022 06:38
@alexytsu alexytsu requested a review from abright October 23, 2022 22:20
Copy link
Contributor

@abright abright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I think we're going to need to revisit some of the state management/reloading stuff later but it's probably best to do that when the dev branch is ready to merge

///
/// Creates an up-to-date view of the actor state where necessary to generate the values
pub fn mint_return(&mut self, intermediate: MintIntermediate, cid: Cid) -> Result<MintReturn> {
self.reload_if_changed(cid)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will keep an eye on this pattern.

It feels like making sure state is reloaded is a safety/correctness concern that should be enforced by the library. But agree that it limits opportunities for optimisation (especially if there's other state to take care of)

@alexytsu alexytsu merged commit 95afbc0 into nft-dev Oct 24, 2022
@alexytsu alexytsu deleted the nft/wip-5 branch October 24, 2022 00:45
alexytsu added a commit that referenced this pull request Nov 8, 2022
alexytsu added a commit that referenced this pull request Nov 17, 2022
* NFT Transfers (#130)

* basic transfer logic

* wip

* allow account level delegation

* token-level approvals

* singular to batched state apis (#133)

* rename account-level authorization methods

* use vector of ActorIDs to record operators

* NFT Receiver Hooks (#135)

* add receiver hook support to state level

* batch for minting

* structure tests with logical blocks

* generate return value from current state (#138)

* State Invariant Checker for NFTs (#140)

* added a state invariant checker for NFTs

* delete map entries where possible during burn operations

* update toolchain during build jobs

* cargo clippy --fix

* Multi actor integration tests (#141)

* added a state invariant checker for NFTs

* delete map entries where possible during burn operations

* update toolchain during build jobs

* cargo clippy --fix

* metadataid to metadata

* separate traits for testing frc46 and nft testing fixtures

* rename test_actor to frc46_test_actor

* test nft actor

* actor to actor transfer tests

* fix bug in operator burn

* NFT Integration Tests (#143)

* separate testing modules into separate files

* mint to alice - alice rejects

* mint to token contract itself

* alice mints to herself - burns

* mint to alice, hook transfers to bob

* alice mints to bob who rejects

* token contract transfer tests

* two actor tests completed

* fix dependencies

* don't link nft_actor into integration tests

* NFT improvements (#145)

* add OwnerOf

* annotate tests with post-run state

* make metadata a string instead of a cid

* use bitfield to store operators

* temporary fix for yanked funty1.2.0
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.

None yet

2 participants