Skip to content

Conversation

@Brechtpd
Copy link
Contributor

@Brechtpd Brechtpd commented Sep 8, 2020

Optimized most frequent paths. In normal cases this only saves ~0.5%-1.0% of gas. Calls with a lot of data will save more gas, but probably quite infrequent.

I do feel like the compiler should be able to do this so we don't have to worry about this. Maybe I'll make an issue on the solidity github.

@Brechtpd Brechtpd requested a review from dong77 September 8, 2020 15:16
returns (bytes memory)
{
return Wallet(wallet).transact(uint8(1), to, value, data);
bytes memory callData = abi.encodeWithSelector(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we also use fastCallAndVerify in transactDelegateCall and transactStaticCall>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should we change memory to calldata as in transactDelegateCall without the function's body change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes transactCall is used with a calldata data from the caller; sometimes the data is assembled in memory.

@dong77
Copy link
Contributor

dong77 commented Sep 9, 2020

If the saving of gas is only ~0.5%-1.0%, we probably want to skip it :)

@Brechtpd
Copy link
Contributor Author

Brechtpd commented Sep 9, 2020

If the saving of gas is only ~0.5%-1.0%, we probably want to skip it

I agree. We can keep the changes to calldata because why not, but the replacements for the calls isn't worth it for the smart wallet use cases where the data isn't very large.

@dong77
Copy link
Contributor

dong77 commented Sep 9, 2020

If the saving of gas is only ~0.5%-1.0%, we probably want to skip it

I agree. We can keep the changes to calldata because why not, but the replacements for the calls isn't worth it for the smart wallet use cases where the data isn't very large.

Great, let's do that.

@Brechtpd
Copy link
Contributor Author

Brechtpd commented Sep 9, 2020

Removed use of the new calls in the smart wallet, but left the functions in the library because they are actually useful in other scenario's.

@Brechtpd Brechtpd requested a review from dong77 September 9, 2020 14:02
@Brechtpd Brechtpd merged commit 0627946 into master Sep 10, 2020
@Brechtpd Brechtpd deleted the hebao-call branch September 10, 2020 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants