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

WIP: test code hash #241

Merged
merged 8 commits into from
Aug 6, 2021
Merged

WIP: test code hash #241

merged 8 commits into from
Aug 6, 2021

Conversation

fulldecent
Copy link
Collaborator

@fulldecent fulldecent commented Feb 6, 2020

});

const logs = sendsToSelfOnConstruct.logs;
ctx.is(logs.events.Received, undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Deploy is a bit different then other transactions. It returns an object with an instance which is an instance of the deployed contract and receipt with information about the deployment. Receipt is what you should look at. If receipt does not contain what you need try updating to the latest specron version: 0.16.0

@fulldecent
Copy link
Collaborator Author

@MoMannn Updated to latest, using receipt. Have one little issue with testing the event.

@xpepermint
Copy link
Member

@fulldecent What kind of issue?


const receipt = sendsToSelfOnConstruct.receipt;
console.log(receipt.events.Received());
ctx.not(receipt.events.Received, undefined); // I want to confirm here that there is only one event (the mint(), and not the safeTransferFrom)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have all the emmited events here. So even if you are expecting same kind of event. Lets say Transfer you will have an array of Transfer events here and can check if there is only one and how it is constructed.

@xpepermint xpepermint requested a review from MoMannn December 23, 2020 19:32
@xpepermint
Copy link
Member

@fulldecent what should we do with this?

@fulldecent
Copy link
Collaborator Author

We should fix this up and merge. Can bill the effort to our client because 721 is being used on that other blockchain.

And this PR is useful for blockchains other than Ethereum Mainnet.

@xpepermint
Copy link
Member

@fulldecent status?

@MoMannn
Copy link
Collaborator

MoMannn commented Aug 5, 2021

@fulldecent
What is the point of this? You are only adding a test where you send yourself a token? What is the expected result/meaning for this?

Don't know how to fix / make this work if I don't know what the context is for this...

@fulldecent
Copy link
Collaborator Author

This test is checking whether a token contract, which sends itself a token during contract creation, will trigger onERC721Received.

This behavior is expected to be different based on using EXTCODESIZE and EXTCODEHASH (pre- and post- Homestead versions).

The implementation here supports Ethereum (EXTCODEHASH) but the implementation in 0xcert Framework supports pre-Homestead (notably, other chains pre Homestead) so I wanted to make a test that illustrates this difference.

@MoMannn
Copy link
Collaborator

MoMannn commented Aug 6, 2021

@fulldecent I fixed the check so it verifies there is no Received event emitted. There are only 2 Transfer events (first for creating the token, the second for sending the token to yourself.)

If this is the expected result we can merge.

@xpepermint
Copy link
Member

@fulldecent please verify.

@fulldecent
Copy link
Collaborator Author

Thank you, LGTM

@xpepermint xpepermint merged commit e5ceb49 into master Aug 6, 2021
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.

3 participants