Skip to content
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

Remove bridge struct #855

Closed
wants to merge 6 commits into from
Closed

Remove bridge struct #855

wants to merge 6 commits into from

Conversation

Primata
Copy link
Contributor

@Primata Primata commented Nov 14, 2024

Summary

  • RFCs: Link to RFC, Link to RFC, or $\emptyset$.
  • Categories: any of protocol-units, networks, scripts, util, cicd, or misc.

Implementation of no struct bridge issued by #846 after getting exorbitantly expensive bridging.

Changelog

removes struct and adapts calls so it's fully dependent on hashing arguments.

Testing

cd to bridge contracts
forge test NativeBridgeMOVETest --gas-report

| src/NativeBridgeCounterpartyMOVE.sol:NativeBridgeCounterpartyMOVE contract |                 |       |        |       |         |
|----------------------------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                                            | Deployment Size |       |        |       |         |
| 659978                                                                     | 2834            |       |        |       |         |
| Function Name                                                              | min             | avg   | median | max   | # calls |
| bridgeTransfers                                                            | 554             | 554   | 554    | 554   | 3       |
| completeBridgeTransfer                                                     | 944             | 10410 | 944    | 76518 | 8       |
| initialize                                                                 | 92870           | 92870 | 92870  | 92870 | 3       |
| lockBridgeTransfer                                                         | 5243            | 6003  | 5243   | 10568 | 14      |


| src/NativeBridgeInitiatorMOVE.sol:NativeBridgeInitiatorMOVE contract |                 |       |        |       |         |
|----------------------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                                      | Deployment Size |       |        |       |         |
| 786068                                                               | 3417            |       |        |       |         |
| Function Name                                                        | min             | avg   | median | max   | # calls |
| initialize                                                           | 92904           | 92904 | 92904  | 92904 | 3       |
| initiateBridgeTransfer                                               | 69223           | 69223 | 69223  | 69223 | 1       |
| setCounterpartyAddress                                               | 24702           | 24702 | 24702  | 24702 | 3       |
| withdrawMOVE                                                         | 37552           | 37552 | 37552  | 37552 | 1       |

@franck44
Copy link

This implementation will make it very difficult to process refunds as we don't save the details of a transaction.
This is discussed in issue 846.

The savings are marginal and introduce more problems than they solve.

@Primata
Copy link
Contributor Author

Primata commented Nov 15, 2024

This implementation will make it very difficult to process refunds as we don't save the details of a transaction.

This is discussed in issue 846.

The savings are marginal and introduce more problems than they solve.

We do have the details of the transaction directly from eth_getLogs or if we choose to store on move storage only.
70% savings is far from marginal.

@0xmovses
Copy link
Contributor

Closing as these contracts are removed for the Trusted Relayer, thanks for the optimisations here .

@0xmovses 0xmovses closed this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants