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

feat: generate inversions of abi.encodePacked() for StakingMessages.unpack*() #482

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
8777f88
perf: `SetSubnetValidatorWeightMessage` (un)packing gas reductions
ARR4N Aug 6, 2024
8b0557a
chore: `forge fmt`
ARR4N Aug 6, 2024
d5f52da
chore: `solhint-disable` assembly checks + some commentary
ARR4N Aug 6, 2024
1c94c27
feat: generated code for `abi.encodePacked()` inversion
ARR4N Aug 9, 2024
c89bd4a
fix: `solc` pragma `^0.8.0` unless using `mcopy` then `^0.8.25`
ARR4N Aug 9, 2024
dac07fc
fix: remove unused variable when dynamically sized at end
ARR4N Aug 9, 2024
c3ac02a
chore: add copyright notices
ARR4N Aug 9, 2024
e264572
chore: placate the linters... ssshhhhh
ARR4N Aug 9, 2024
7f0bd2e
perf: `SetSubnetValidatorWeightMessage` (un)packing gas reductions
ARR4N Aug 6, 2024
9890cca
chore: `forge fmt`
ARR4N Aug 6, 2024
721b63e
chore: `solhint-disable` assembly checks + some commentary
ARR4N Aug 6, 2024
ef87566
feat: generated code for `abi.encodePacked()` inversion
ARR4N Aug 9, 2024
c9e28b6
fix: `solc` pragma `^0.8.0` unless using `mcopy` then `^0.8.25`
ARR4N Aug 9, 2024
1c2ad1e
fix: remove unused variable when dynamically sized at end
ARR4N Aug 9, 2024
7f98e2a
chore: add copyright notices
ARR4N Aug 9, 2024
ee4bbf6
chore: placate the linters... ssshhhhh
ARR4N Aug 9, 2024
e6cf4e5
Merge branch 'arr4n/staking-messages' of github.com:ava-labs/teleport…
ARR4N Aug 9, 2024
3bb7bb6
chore: remove obsolete test of equivalence
ARR4N Aug 9, 2024
34f629f
refactor: `abi.encodePacked()` and `Unpack.unpack_...()` all the things
ARR4N Aug 9, 2024
f32bbcf
chore: make Slither keep quiet
ARR4N Aug 9, 2024
0272bde
chore: disable `line-length-limiter` on flag usage
ARR4N Aug 9, 2024
aed84c7
feat: use literal "dyn" string in `unpackgen` flag
ARR4N Aug 9, 2024
a279844
Merge branch 'staking-contract' into arr4n/staking-messages
ARR4N Aug 12, 2024
de4da9f
Merge branch 'staking-contract' into arr4n/staking-messages
ARR4N Sep 6, 2024
aab0874
feat: `pragma solidity` version override
ARR4N Sep 6, 2024
f4a773c
fix: apply explicit `solc` version to generated tests
ARR4N Sep 6, 2024
1bfcf4b
doc: clarify allowable interchange of `bytes<N>` and `uint<8N>` types
ARR4N Sep 6, 2024
78a1f47
refactor: only change `pack*()` methods to `abi.encodePacked()`
ARR4N Sep 6, 2024
e13cabc
refactor: change `unpack*()` methods to use generated code
ARR4N Sep 6, 2024
a19b841
chore: regenerate ABI bindings
ARR4N Sep 6, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

134 changes: 134 additions & 0 deletions contracts/staking/Unpack.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
// (c) 2024, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.
// SPDX-License-Identifier: UNLICENSED
// slither-disable-next-line solc-version
pragma solidity "0.8.25"; // solhint-disable-line compiler-version

// GENERATED CODE - Do not edit

// solhint-disable no-inline-assembly
// slither-disable-start assembly
// Mixed-case can't apply to numbers and an underscore is the most natural separator
// solhint-disable func-name-mixedcase

library Unpack {
/// @dev Thrown if the input buffer to an unpack function is of the wrong length.
error IncorrectInputLength(uint256 expected, uint256 actual);

Choose a reason for hiding this comment

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

how strongly do you feel about custom errors? We previously reverted from custom errors back to require statements, because we felt some tooling weren't updated to custom errors and lacked in debugging

Copy link
Author

Choose a reason for hiding this comment

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

Which tooling lacks support?

I think they're superior to string errors:

  1. Don't need to store the entire string in the contract code
  2. Can be parameterised to aid debugging and allow precise path detection in tests
  3. Avoid change-detector tests

Choose a reason for hiding this comment

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

we reverted to require statements over a year ago, so probably need to revisit this. I think back then some combination of forge errors, block explorers, and e2e testing just showed the hex error signature. This still seems like the case for our e2e test code, but there's likely a workaround we could do.

I am also leaning towards custom errors now, but think we should tackle this in a separate issue, and still conform to require to match the rest of the repo.

Copy link
Author

Choose a reason for hiding this comment

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

It's not a hill I'll die on, but something I feel I should push back on and ask "are you sure?" before I implement a toggle between the two.

Independent benchmarking of both deployment and usage gas.

Where is the problematic e2e test code?

Copy link
Author

Choose a reason for hiding this comment

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

As it currently stands, these reverts can never occur because I left the original length checks in place for the time being.

Choose a reason for hiding this comment

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

This folder holds the e2e tests I'm referring to https://github.com/ava-labs/teleporter/tree/main/tests/flows. This is also not a hill I'll die on, since I'm also leaning towards custom errors at this point, but wasn't sure to defer to a separate issue and changing all the require statements in the repo together so we can keep them consistent for now.


/// @dev Thrown if the input buffer to an unpack function with dynamically sized output is too short.
error InsufficientInputLength(uint256 min, uint256 actual);

/**
* @notice Inverts `abi.encodePacked()` for the specific return types.
* @dev The originally packed inputs MUST be of the same byte lengths as those returned by this function, in the
* precise order, otherwise the output is undefined. `bytes<N>` and `uint<8N>` are interchangeable.
* @dev This function takes ownership of and corrupts the `input` parameter to avoid memory expansion for the
* returned `bytes memory`. The returned array is owned by the caller.
* @param input Buffer returned by `abi.encodePacked()`
*/
// slither-disable-next-line naming-convention
function unpack_2_4_32_32_8_Dyn_8_Destructive(bytes memory input)
internal
pure
returns (
bytes2 v0,
bytes4 v1,
bytes32 v2,
bytes32 v3,
bytes8 v4,
bytes memory v5,
bytes8 v6
)
{
if (input.length < 86) {
revert InsufficientInputLength(86, input.length);
}

assembly ("memory-safe") {
v0 := mload(add(input, 0x20))
v1 := mload(add(input, 0x22))
v2 := mload(add(input, 0x26))
v3 := mload(add(input, 0x46))
v4 := mload(add(input, 0x66))
let length := mload(input)
let dynLength := sub(length, 86)
v5 := add(input, 78)
mstore(v5, dynLength)
let end := add(input, add(length, 0x20))
v6 := mload(sub(end, 0x08))
}
}

/**
* @notice Inverts `abi.encodePacked()` for the specific return types.
* @dev The originally packed inputs MUST be of the same byte lengths as those returned by this function, in the
* precise order, otherwise the output is undefined. `bytes<N>` and `uint<8N>` are interchangeable.
* @param input Buffer returned by `abi.encodePacked()`
*/
// slither-disable-next-line naming-convention
function unpack_2_4_32_1(bytes memory input)
internal
pure
returns (bytes2 v0, bytes4 v1, bytes32 v2, bytes1 v3)
{
if (input.length != 39) {
revert IncorrectInputLength(input.length, 39);
}

assembly ("memory-safe") {
v0 := mload(add(input, 0x20))
v1 := mload(add(input, 0x22))
v2 := mload(add(input, 0x26))
v3 := mload(add(input, 0x46))
}
}

/**
* @notice Inverts `abi.encodePacked()` for the specific return types.
* @dev The originally packed inputs MUST be of the same byte lengths as those returned by this function, in the
* precise order, otherwise the output is undefined. `bytes<N>` and `uint<8N>` are interchangeable.
* @param input Buffer returned by `abi.encodePacked()`
*/
// slither-disable-next-line naming-convention
function unpack_2_4_32_8_8(bytes memory input)
internal
pure
returns (bytes2 v0, bytes4 v1, bytes32 v2, bytes8 v3, bytes8 v4)
{
if (input.length != 54) {
revert IncorrectInputLength(input.length, 54);
}

assembly ("memory-safe") {
v0 := mload(add(input, 0x20))
v1 := mload(add(input, 0x22))
v2 := mload(add(input, 0x26))
v3 := mload(add(input, 0x46))
v4 := mload(add(input, 0x4e))
}
}

/**
* @notice Inverts `abi.encodePacked()` for the specific return types.
* @dev The originally packed inputs MUST be of the same byte lengths as those returned by this function, in the
* precise order, otherwise the output is undefined. `bytes<N>` and `uint<8N>` are interchangeable.
* @param input Buffer returned by `abi.encodePacked()`
*/
// slither-disable-next-line naming-convention
function unpack_2_4_32_8(bytes memory input)
internal
pure
returns (bytes2 v0, bytes4 v1, bytes32 v2, bytes8 v3)
{
if (input.length != 46) {
revert IncorrectInputLength(input.length, 46);
}

assembly ("memory-safe") {
v0 := mload(add(input, 0x20))
v1 := mload(add(input, 0x22))
v2 := mload(add(input, 0x26))
v3 := mload(add(input, 0x46))
}
}
}
Loading