openzeppelin-contracts

openzeppelin-contracts copied to clipboard

It might be nit, but you define _balances in your ERC1155 implementation accordingly:

mapping(uint256 id => mapping(address account => uint256)) private _balances;

but the balanceOf function uses the (switched) EIP-1155-compliant way as function arguments:

function balanceOf(address account, uint256 id) public view virtual returns (uint256) {
    return _balances[id][account];
}

I would propose to change _balances to:

mapping(address account => mapping(uint256 id => uint256)) private _balances;

in order to avoid confusion and to keep consistency with the balanceOf function. My guess, why it has been implemented like this in OZ, is the official EIP implementation here.

Hey @pcaversaccio,

Took a quick look at the code because I agree it feels weird. So far I agree with the rationale but changing the order of the mapping is a breaking change for upgradeable contracts. I don't think we can fix that until 6.0 and it doesn't harm.

Hey @pcaversaccio,

Took a quick look at the code because I agree it feels weird. So far I agree with the rationale but changing the order of the mapping is a breaking change for upgradeable contracts. I don't think we can fix that until 6.0 and it doesn't harm.

Yeah it doesn't harm and you guys can patch it whenever the timing is ready. I just think it's kinda confusing and should be patched one day at least.

Sounds good to me, I'm labeling this as a breaking change so we take a look back when we work on a new version.

Thanks for raising it!