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

Getting the parent of a child #52

Merged
merged 17 commits into from
Apr 22, 2023
Merged

Getting the parent of a child #52

merged 17 commits into from
Apr 22, 2023

Conversation

Szegoo
Copy link
Contributor

@Szegoo Szegoo commented Mar 12, 2023

Introduces a new storage structure that keeps track of the parent of a child.

This is required for the new query for getting the parent of a child.

Closes: #12

@Szegoo Szegoo marked this pull request as ready for review March 13, 2023 18:55
@Szegoo
Copy link
Contributor Author

Szegoo commented Mar 13, 2023

From my understanding, there is no way at the moment to iterate over a Mapping therefore I introduced a new mapping that will keep track of the parents of children. Also, would it be useful to track parents of pending children?

@Maar-io @ilionic

@ilionic ilionic requested review from boyswan and Maar-io March 16, 2023 09:41
@Maar-io
Copy link
Contributor

Maar-io commented Mar 17, 2023

@bobo-k2 can you please take a look and comment

Copy link
Contributor

@Maar-io Maar-io left a comment

Choose a reason for hiding this comment

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

We discussed previously that it is hard to track parent NFT, but in case of nesting, the parent-NFT-address is the owner of the child NFT.
And childNFT can have only one parent

crates/nesting/src/lib.rs Outdated Show resolved Hide resolved
@bobo-k2
Copy link
Contributor

bobo-k2 commented Mar 20, 2023

I like the idea, because this is the feature I am missing from current implementation. I agree with @Maar-io. Calling add_chilld is called on parent and you code will update children_parent mapping on parent. Also, please add some tests.

@Szegoo Szegoo marked this pull request as draft March 26, 2023 15:31
crates/nesting/src/lib.rs Outdated Show resolved Hide resolved
crates/rmrk/src/query.rs Outdated Show resolved Hide resolved
@Szegoo
Copy link
Contributor Author

Szegoo commented Apr 7, 2023

@Maar-io @bobo-k2 It seems like calling any query from the query.js file in the integration tests results in the following error:

Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)

Should I test the new query with an integration test, seems like we are not testing any other queries.

@boyswan
Copy link
Contributor

boyswan commented Apr 13, 2023

@Maar-io @bobo-k2 It seems like calling any query from the query.js file in the integration tests results in the following error:

Error: the object {
  "issue": "OUTPUT_IS_NULL"
} was thrown, throw an Error :)

Should I test the new query with an integration test, seems like we are not testing any other queries.

Do you have an example of a query being called?

@Szegoo
Copy link
Contributor Author

Szegoo commented Apr 13, 2023

Do you have an example of a query being called?

@boyswan You mean do I have an example of the get parent query being called, or? For me calling any query in the integration test results in the above-mentioned error. I am not sure if others are facing the same problem. This is the way I make the query:

// code inside the nesting integration test.
// -- snip --
const _ = await child.query.getParentOfChild([child.address, { u64: 1}]);
// ^^^^ Making this call results in an error.
// -- snip --

@bobo-k2
Copy link
Contributor

bobo-k2 commented Apr 14, 2023

"OUT

@Szegoo Mario and I debugged similar issue last week. Maybe I can help to find root cause of the problem
Open rmrk-ink/node_modules/@727-ventures/typechain-types/dist/src/query.js and set breakpoint in line 203
image
Run test in debug mode and when breakpoint is reached in debug console type something like (I am writing this from top of my head so It may not work at first)
console.log(result.asErr.registry.findMetaError(result.asErr.asModule);

@boyswan
Copy link
Contributor

boyswan commented Apr 14, 2023

I did some digging and it seems like the issue lies with how the query functions call their storage (was using MintingRef as a way to call internal storage instead of directly accessing it).

It looks like this behaviour has broken since ink 4.0, which will require some changes to how the Query trait works. We can make a separate issue/PR for this, as it's out of the scope of this feature!

@Szegoo Szegoo marked this pull request as ready for review April 14, 2023 08:33
@Szegoo
Copy link
Contributor Author

Szegoo commented Apr 14, 2023

I've created a separate issue for this: #64

@Maar-io
Copy link
Contributor

Maar-io commented Apr 14, 2023

you get OUTPUT_IS_NULL when your contract crashes.

The way to see what was the error code is to make breakpoint on the line you posted and check something like
const mod = dispatchError.asModule;
const error = dispatchError.registry.findMetaError(mod);

@Szegoo
Copy link
Contributor Author

Szegoo commented Apr 14, 2023

After running in debug mode and some logging I got the following error message:

A call tried to invoke a contract that is flagged as non-reentrant.

https://github.com/paritytech/substrate/blob/3f8dc9ed594fdba455d0bf6780768e7176188514/frame/contracts/src/lib.rs#L829-L832
Seems to me that @boyswan was correct.

@boyswan
Copy link
Contributor

boyswan commented Apr 15, 2023

Ahh of course - makes total sense now! So all we need to do is allow reentrancy for these calls. For example:

let parent_collection = PSP34Ref::owner_of_builder(&child_collection, child_id)
    .call_flags(ink::env::CallFlags::default().set_allow_reentry(true))
    .try_invoke();

@Szegoo
Copy link
Contributor Author

Szegoo commented Apr 15, 2023

Oh, I got a bit confused, but I understand now :)

crates/nesting/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@boyswan boyswan left a comment

Choose a reason for hiding this comment

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

With clone fix, LGTM

crates/rmrk/src/query.rs Outdated Show resolved Hide resolved
@Szegoo Szegoo requested a review from Maar-io April 20, 2023 15:16
Copy link
Contributor

@Maar-io Maar-io left a comment

Choose a reason for hiding this comment

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

LGTM

@Szegoo Szegoo merged commit c03a3a5 into rmrk-team:main Apr 22, 2023
3 of 4 checks passed
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.

Missing ownerOfChild
4 participants