V2.1 #317
V2.1 #317
Conversation
| */ | ||
| mapping(address => mapping(bytes32 => AuthorizationState)) | ||
| private _authorizationStates; |
There was a problem hiding this comment.
I've verified that this change is safe and backwards-compatible. EVM treats 0 as false and non-zero values as true.
There was a problem hiding this comment.
What about the storage? Does solidity do any packing, or does it promise to always use a full slot for mappings? Thinking of what would happen if the old value was 2, but maybe its only reading the least significant bit for a bool for example.
What did you do to verify? Have you tried out an upgrade manually? This is hard to test, since the tests won't have access to the old code, so I want to be really sure.
There was a problem hiding this comment.
I tested it manually. Mappings always use full slots unless struct values are used, and the position is determined via a keccak256 hash of the key (k) and the position of the mapping itself (p): slot(authorizationStates[k1][k2]) = keccak256(k2 . keccak256(k1 . p). I also tested the casting via calling SLOAD directly:
pragma solidity 0.6.12;
contract Foo {
enum AuthorizationState { Unused, Used, Canceled }
AuthorizationState public a = AuthorizationState.Canceled;
function authorizationState() external view returns (AuthorizationState) {
return a; // returns 2
}
function authorizationStateBool() external view returns (bool) {
bool x;
assembly {
x := sload(0)
}
return x; // returns true
}
}
|
|
||
| /** | ||
| * @title FiatToken V2 | ||
| * @notice ERC20 Token backed by fiat reserves, version 2 | ||
| */ | ||
| contract FiatTokenV2 is FiatTokenV1_1, GasAbstraction, Permit { | ||
| bool internal _initializedV2; |
There was a problem hiding this comment.
I've verified that this change is safe and backwards compatible.
There was a problem hiding this comment.
Similar to above, are you sure solidity didn't pack the storage that might no be compatible with the new type? How did you verify that it is safe?
There was a problem hiding this comment.
The uint8 will occupy the exact same number of bytes (1 byte) in the storage in the exact same position, and the previous value of true will become 1.
| */ | ||
| mapping(address => mapping(bytes32 => AuthorizationState)) | ||
| private _authorizationStates; |
There was a problem hiding this comment.
What about the storage? Does solidity do any packing, or does it promise to always use a full slot for mappings? Thinking of what would happen if the old value was 2, but maybe its only reading the least significant bit for a bool for example.
What did you do to verify? Have you tried out an upgrade manually? This is hard to test, since the tests won't have access to the old code, so I want to be really sure.
|
|
||
| /** | ||
| * @title FiatToken V2 | ||
| * @notice ERC20 Token backed by fiat reserves, version 2 | ||
| */ | ||
| contract FiatTokenV2 is FiatTokenV1_1, GasAbstraction, Permit { | ||
| bool internal _initializedV2; |
There was a problem hiding this comment.
Similar to above, are you sure solidity didn't pack the storage that might no be compatible with the new type? How did you verify that it is safe?
| if (lockedAmount > 0) { | ||
| _transfer(address(this), lostAndFound, lockedAmount); | ||
| } | ||
| blacklisted[address(this)] = true; |
There was a problem hiding this comment.
Should we emit a Blacklisted event?
There was a problem hiding this comment.
I didn't think it was necessary, also was worried that some unsophisticated people may report it as CENTRE blacklisting yet another address and cause some false alarm.
| await expectRevert( | ||
| fiatToken.initializeV2_1(lostAndFound, { from: fiatTokenOwner }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Shoudl we have some basic transfer/mint tests to/from other addresses to make sure they still work after the contract is blacklisted?
There was a problem hiding this comment.
Sure, I can add those.
| */ | ||
| function authorizationState(address authorizer, bytes32 nonce) | ||
| external | ||
| view | ||
| returns (AuthorizationState) | ||
| returns (bool) |
There was a problem hiding this comment.
If a contract was compiled against the old version, what does this map to in terms of the enum? Does true get interpreted as Used? which would be nice
There was a problem hiding this comment.
The enum gets compiled to an integer with possible values of 0, 1, and 2. When read as "bool", the value of 0 gets interpreted as false and others get interpreted as true.
There was a problem hiding this comment.
Yeah that makes sense. I was thinking of the inverse - when a bool comes back but the caller reads it as an AuthorizationState. I think if true comes back, it should be as 1, which looks like it would map to AuthorizationState.Used?
There was a problem hiding this comment.
@eztierney Yes that's correct:
pragma solidity 0.6.12;
interface Legacy {
enum AuthorizationState { Unused, Used, Canceled }
function authorizationState() external view returns (AuthorizationState);
}
contract New {
bool public state = true;
function authorizationState() external view returns (bool) {
return state;
}
function legacyAuthorizationState() external view returns (Legacy.AuthorizationState) {
Legacy.AuthorizationState x = Legacy(address(this)).authorizationState();
return x; // returns 1 (Legacy.AuthorizationState.Used)
}
}
Changes:
receiveWithAuthorizationfunction, removes{approve,increaseAllowance,decreaseAllowance}WithAuthorizationfunctions. According to the block explorer, onlytransferWithAuthorizationandtransferMultipleWithAuthorizationshave been used so far (139 transactions) on the Mainnet, so it should be safe to remove the said functions.