Extending ERC1155 with approvals by amount #5216
Conversation
All tests passed; auto-merging...(pass) eip-5216.md
(pass) assets/eip-5216/ERC1155ApprovalByAmount.sol
|
LICENSE and package-lock.json deleted |
|
The commit d3f7827 (as a parent of 9523834) contains errors. Please inspect the Run Summary for details. |
There was a problem hiding this comment.
The goal of the reference implementation is to give readers a rough idea of what an implementation of this token might look like in as little code as possible so it remains relevant to the standard at hand and isn't bogged down with supplemental stuff. This means that you should not include a complete project with dependencies, a build system, test infrastructure, etc. as that sort of content is much better kept in some repository that is actively maintained.
In this case, I recommend removing everything in assets/eip-5216 except ERC1155ApprovalByAmount.sol, IERC1155ApprovalByAmount, and maybe ExampleToken.sol. I would also combine them all into one file rather than spreading them out over several files, and in place of the dependency on @openzeppelin/* just put ERC1155.sol/IERC1155.sol and leave it as an exercise for the reader to find/implement ERC-1155. You can also completely remove the Ownable.sol reference as that is out of scope of this standard.
For the test cases, it is valuable to include test cases but they should ideally be in a form that is somewhat platform agnostic. An ideal set of test cases is a JSON file or something that is an array of function, inputs, expected_outputs or similar. You may want to include some mechanism for representing postcondition checking and/or event checking as well if appropriate. What isn't necessary is to have a fully executable test suite written in a particular language and doing so can make it harder for implementers to adapt to their own infrastructure (which may be wildly different from yours).
|
Oops it appears there might have been a bug. |
|
Oh. There's a problematic branch protection setting. Please disable the "require conversation resolution" option @lightclient. |
|
@Pandapip1 it isn't enabled. |
|
Wow.. thanks guys |
ivanmmurciaua
requested review from
lightclient and
SamWilsn
and removed request for
SamWilsn and
lightclient
* Create eip-approval_by_amount.md * assets added to eip-5216 * MIT License, package-lock.json deleted and rename md to EIP number * Issues with author names, spell check and EIP standard bot * Removed all assets and README updated. * License and other issues
Approval by amount extension