-
Notifications
You must be signed in to change notification settings - Fork 916
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
Optimize Personal Edition #25
Conversation
contracts/GnosisSafe.sol
Outdated
uint256 public nonce; | ||
uint8 public threshold; |
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.
The declaration of the threshold was moved to ensure that it is not combined with the masterCopy address
pure | ||
returns (uint256) | ||
{ | ||
return 2; |
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.
Not sure if these methods should be declared here or in the master copy
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 the intention was to implement them in Proxy.sol.
What was the motivation to change the extension behaviour? |
In my opinion it is more flexible, but it was just an example implementation. I commented on the issue for the extension change #14 |
return startGas - gasleft(); | ||
} | ||
|
||
function checkHash(bytes32 hash, uint8[] v, bytes32[] r, bytes32[] s) |
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.
Add natspec here?
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 will change it to internal, no need to expose that in the abi
public | ||
{ | ||
uint256 startGas = gasleft(); | ||
require(address(this).balance - value >= totalGasCosts(safeTxGas) * gasPrice); |
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 this check is really helpful. This check allows to let a transaction fail early in case the transaction service was front run by another service or safe owner. In this attack scenario the attacker could easily obfuscate value transfers using multisend transactions to make the transaction not fail.
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.
True, I will remove it
checkHash(getTransactionHash(to, value, data, operation, safeTxGas, gasPrice, nonce), v, r, s); | ||
// Increase nonce and execute transaction. | ||
nonce++; | ||
require(gasleft() - PAYMENT_GAS_COSTS >= safeTxGas); |
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.
Is this check really necessary? The tx service should be able to verify that this require statement will always be satisfied at the time the transaction is sent.
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.
This is so that the service cannot set a too low total gas that makes the user transaction fail, but still allow a successful transfer of funds
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.
Oh right, because execute(to, value, data, operation, safeTxGas) wont revert if safeTxGas < gasleft() right?
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.
yes, it might still use up the gas so the final payment would fail ... but the check makes this more secure
pure | ||
returns (uint256) | ||
{ | ||
return executionGas + PAYMENT_GAS_COSTS + BASE_TX_GAS_COSTS + msg.data.length * DATA_BYTE_GAS_COSTS; |
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.
The tx service has the possibility to add a lot of useless payload to the transaction, to let the user pay more than necessary. Maybe we should include the data.length in the signature.
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.
In this case we maybe should precalculate the whole data costs
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.
Agreed, this would make sense.
delegate(msg.data, true); | ||
} | ||
|
||
function delegate(bytes _calldata, bool returnData) |
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.
Why not always retrunData == true? I would remove this argument as the constructor case a single time use case.
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.
If we copy the delegate call logic in the constructor the one time costs might increase not sure how much the per transaction gas increases because of that, will check
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.
hmm gas costs are exactly the same :D
confirmationCount++; | ||
} | ||
} | ||
setupOwners(_owners, _threshold); |
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.
Constructors of inherited contracts can be called like modifiers (https://github.com/gnosis/MultiSigWallet/blob/master/contracts/MultiSigWalletWithDailyLimit.sol#L30). I guess this won't work for regular functions (not sure)?
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.
not as far as I understand the docs.
contracts/GnosisSafeTeamEdition.sol
Outdated
bool ownerConfirmed = isConfirmed[transactionHash][owner]; | ||
if(owner == msg.sender || ownerConfirmed) { | ||
if (ownerConfirmed) { | ||
isConfirmed[transactionHash][owner] = false; |
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.
For gas refunds?
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.
Yes, without would be cleaner, what do you prefer?
contracts/GnosisSafe.sol
Outdated
} | ||
} | ||
setupOwners(_owners, _threshold); | ||
setupModules(to, data); |
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 could make setupOwners and setupModules internal functions. They can still be called in the Safe setup function and we won't have to set the initialized storage in the modules manager.
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 made setupModules public to have the possibility to just deploy the modulemanager (that was more the case for personal edition module solution). I can take that out and if somebody wants this functionality they have to add a StandaloneModuleManger
that has this functionality
…de them in dataGas
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.
Thanks
Implements:
gas
opcode overnot(0)
#8: Use gas instead of not(0)if
instead ofswitch
in the ProxyFactory contract #7: Replace assembly switch with ifToDos: