erc6551 / reference

ERC-6551 reference implementation
153 stars 50 forks source link

Optimize initcode #21

Closed Vectorized closed 1 year ago

Vectorized commented 1 year ago

Just OCD.

Saves 2 gas. The initcode is still of the same length.

Could use CALLVALUE (i.e. msg.value) instead of PUSH1 00, but PUSH1 00 gives the same length.

Not sure if you want to make createAccount payable and pass in the msg.value into the newly created account (might be convenient for cases where people want to make accounts with some ETH). Payable methods cost less gas, since they don't have the check for zero msg.value. But then we have to handle the case where the account has been already created, possibly sending the msg.value into the account.

Before:

| Opcode     | Mnemonic       | Stack          | Memory                 |
| ---------- | -------------- | -------------- | ---------------------- |
| 3d         | RETURNDATASIZE | 00             |                        |
| 60 runSize | PUSH1 ad       | ad 00          |                        |
| 80         | DUP1           | ad ad 00       |                        |
| 60 offset  | PUSH1 0a       | 0a ad ad 00    |                        |
| 3d         | RETURNDATASIZE | 00 0a ad ad 00 |                        |
| 39         | CODECOPY       | ad 00          | [00..ad): runtime code |
| 81         | DUP2           | 00 ad 00       | [00..ad): runtime code |
| f3         | RETURN         | 00             | [00..ad): runtime code |

After:

| Opcode     | Mnemonic       | Stack          | Memory                 |
| ---------- | -------------- | -------------- | ---------------------- |
| 60 runSize | PUSH1 ad       | ad             |                        |
| 80         | DUP1           | ad ad          |                        |
| 60 offset  | PUSH1 0a       | 0a ad ad       |                        |
| 3d         | RETURNDATASIZE | 00 0a ad ad    |                        |
| 39         | CODECOPY       | ad             | [00..ad): runtime code |
| 60 00      | PUSH1 00       | 00 ad          | [00..ad): runtime code |
| f3         | RETURN         |                | [00..ad): runtime code |
jaydenwindle commented 1 year ago

@Vectorized does this changed the deployed bytecode of the account contract? Or just the constructor logic?

Vectorized commented 1 year ago

@jaydenwindle only the constructor logic.

jaydenwindle commented 1 year ago

@Vectorized I'm hesitant about including this one (although it's very clever!)

There are a lot of existing accounts deployed with the previous registry/initcode. This PR would introduce a breaking change in the way an account address is computed given a registry address. Existing tooling would have to be updated to support both computation methods and some way of determining which one the registry they are interacting with supports. That seems like a lot of effort and potential confusion at the client level, not sure if that is worth saving 2 gas units per deployment.

Thoughts?

Vectorized commented 1 year ago

I see! Backwards compatibility is more important!