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.

Sign up for GitHub

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

Jump to bottom

NFT Dev Branch #142

Merged
merged 8 commits into from
Nov 17, 2022
Merged

NFT Dev Branch #142

merged 8 commits into from
Nov 17, 2022

Conversation

alexytsu
Copy link
Collaborator

@alexytsu alexytsu commented Oct 28, 2022 •
edited
Loading

Long-lived integration branch for landing NFT related work before merging to main

@alexytsu
NFT Transfers (#130)
c1f0df9 
* 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
@alexytsu
NFT Receiver Hooks (#135)
6255f97 
* add receiver hook support to state level

* batch for minting

* structure tests with logical blocks
@alexytsu
generate return value from current state (#138)
28444cf
@alexytsu
State Invariant Checker for NFTs (#140)
5efaaca 
* added a state invariant checker for NFTs

* delete map entries where possible during burn operations

* update toolchain during build jobs

* cargo clippy --fix
@alexytsu
Multi actor integration tests (#141)
9ddfb68 
* 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
@alexytsu
NFT Integration Tests (#143)
770004c 
* 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
anorth
anorth reviewed Nov 14, 2022
View reviewed changes
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking great.

My biggest request is to remove logic from the state layer, making it as dumb as possible, like a nosql database. Then there are a few design questions in discussion where we have updates to make here.

dispatch_examples/greeter/tests/greet.rs Show resolved Hide resolved
frcxx_nft/src/lib.rs Show resolved Hide resolved
frcxx_nft/src/lib.rs Show resolved Hide resolved
frcxx_nft/src/lib.rs Show resolved Hide resolved
frcxx_nft/src/lib.rs Show resolved Hide resolved
frcxx_nft/src/types.rs Show resolved Hide resolved
frcxx_nft/src/types.rs Show resolved Hide resolved
frcxx_nft/src/types.rs Show resolved Hide resolved
frcxx_nft/src/util.rs Show resolved Hide resolved
fvm_actor_utils/src/actor.rs Show resolved Hide resolved
@alexytsu alexytsu marked this pull request as ready for review November 14, 2022 03:25
@coveralls
Copy link

coveralls commented Nov 14, 2022 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 85.937% when pulling 51363eb on nft-dev into 2cb0fc5 on main.

This was referenced Nov 15, 2022
Closed
Closed
anorth
anorth approved these changes Nov 16, 2022
View reviewed changes
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

I see issues for the major things. Approving in order to reduce merge pain and get back to a world of small PRs, but on the understanding we probably need to do another full review once a bunch of design changes and FRC alignment is done.

frcxx_nft/src/state.rs Outdated Show resolved Hide resolved
@alexytsu alexytsu mentioned this pull request Nov 16, 2022
Closed
alexytsu and others added 2 commits November 16, 2022 14:40
@alexytsu
NFT improvements (#145)
1834dc3 
* add OwnerOf

* annotate tests with post-run state

* make metadata a string instead of a cid

* use bitfield to store operators
@alexytsu
temporary fix for yanked funty1.2.0
51363eb
@alexytsu alexytsu merged commit 9c7aa8b into main Nov 17, 2022
@alexytsu alexytsu deleted the nft-dev branch November 17, 2022 06:24
This was referenced Nov 17, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anorth anorth anorth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexytsu @coveralls @anorth