-
Notifications
You must be signed in to change notification settings - Fork 358
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
Add ERC1155 #896
Add ERC1155 #896
Conversation
Update error testing and remove tests failing from erc1155
…iro-contracts into feat/erc1155-#572
hey @ericnordelo do not hesitate to ask me for the review if needed. Let me know when I can start it 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @cloudvenger and @ericnordelo! I think it's ok to leave the docs for a different PR to avoid a gigantic one, but do need to add more tests, especially for the new ERC1155 preset.
I did finish testing the ERC1155 component. Tomorrow I will check the dual case and the preset test modules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @cloudvenger @ericnordelo! I left some feedback :)
@ericnordelo whoops, just saw this^. Don't mind my dual case comments 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're almost there! not sure if we should be adding a mixin now or in another PR
} | ||
|
||
#[test] | ||
fn test_update_wac_from_zero_to_non_zero_camel_receiver() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
Co-authored-by: Martín Triay <[email protected]>
…racts into feat/erc1155-#572
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improvements look great, Eric! I left a few minor suggestions and comments, but this looks just about ready to me
use openzeppelin::tests::token::test_erc1155::{ | ||
assert_only_event_transfer_single, assert_only_event_transfer_batch, | ||
assert_only_event_approval_for_all | ||
}; | ||
use openzeppelin::tests::token::test_erc1155::{ | ||
setup_account, setup_receiver, setup_camel_receiver, setup_account_with_salt, setup_src5 | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it'd make sense to create more granular utils for tests. setup_account
, for example, can be useful in a a few test modules. Just a thought for the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's probably worth it, yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #572
PR Checklist