imp: remove transient storage for payload hash and pass payload bytes instead#57
imp: remove transient storage for payload hash and pass payload bytes instead#57MalteHerrmann wants to merge 11 commits intoadd-hyperlane-adapterfrom
Conversation
…other method that passes the payload bytes directly instead of the hash that's retrieved from the transient storage
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
# Conflicts: # contracts/script/DeployOrbiterGateway.s.sol # contracts/test/TestOrbiterHypERC20.t.sol
|
E2E tests will be fixed on the base branch, has nothing to do with the contracts implementation here. |
0xstepit
left a comment
0xstepit
left a comment
There was a problem hiding this comment.
Great job here @MalteHerrmann! Added a couple of comments but the code looks great
There was a problem hiding this comment.
we should remember to add to codespell for doc the entire doc folder!
|
|
||
| # ------------------------------ | ||
| # Testing | ||
| # Linting |
There was a problem hiding this comment.
nit: can we align the section heading with the main Makefile 🙏
| /// bridging mechanisms (e.g. IBC, CCTP). | ||
| /// | ||
| /// TODO: make upgradeable | ||
| /// TODO: make upgradeable? |
There was a problem hiding this comment.
we should move it to a linear ticket
| /// @notice Creates a new instance of the OrbiterGateway contract. | ||
| /// @param _domain The Hyperlane domain of the Noble network, which is the destination domain for Orbiter transfers. | ||
| constructor(uint32 _domain) { | ||
| destinationDomain = _domain; |
There was a problem hiding this comment.
why are we passing it instead of setting it as a const? For mainnet and testnet?
| /// @param _amount The amount of tokens to transfer. | ||
| /// @param _payload The payload passed along with the asset transfer. | ||
| /// @return messageID The ID of the dispatched Hyperlane message. | ||
| function sendForwardedTransfer( |
There was a problem hiding this comment.
In CCTP we call it WithOrbiterPayload. It would be nice to have parity
| // We only want to handle transfers with a non-zero payload. | ||
| if (_payload.length == 0) { | ||
| revert EmptyPayload(); |
There was a problem hiding this comment.
Isn't better to have this check in the public method? This way we'd avoid a bunch of useless computation
There was a problem hiding this comment.
Mailbox, Messange, and ITransparentUpgradableProxy are not used
There was a problem hiding this comment.
There are variables in the constant section: DECIMALS, SCALE, ...
|
I would also add the formatting config for contracts, in the CCTP ones I have this: [fmt]
multiline_func_header = "attributes_first"
line_length = 100
number_underscore = "thousands"
bracket_spacing = true
wrap_comments = true |
2 participants
This PR adjusts the contract implementation to avoid storing a payload hash in transient storage and then retrieving it within
_transferRemotebut instead is adding a new external methodtransferRemoteWithPayloadthat is enabling to pass the full payload (or a compressed version) as a function argument.