refactor initialization by larskuhtz · Pull Request #13 · kadena-io/foundry-chainweb

larskuhtz · 2025-09-08T06:00:09Z

[This PR currently builds on #12 and will be rebased when #12 is merged.]

It seems that calling selectFork in a constructor of a script or test leads to undefined and inconsistent behavior when switching between chains/forks.

This PR builds fixes this by refactoring the code such that initialization of chains is delayed until to the first call of switchChain.

Both usage patterns that are supported in the current version are also supported by this PR:

Explicit setup and initialization of Chainweb chains in a setUp() of a script or test or
Implicit setup and initialization of Chainweb by inheriting form ChainwebTest or ChainwebScript.

The code could be simplified considerably if we decided to drop the second approach to initialization and instead always require explicit setup via a setUp() function.

[The PR also includes code for mocking the SHA512/256 precompile that is available in the EVM sandbox and testnet networks. The precompile code needs more work and that is the reason why this PR is still in draft state. I plan to move that change into a separate PR]

hswopeams · 2025-09-08T09:40:17Z

run forge fmt at the top level. This should take care of the current CI failures.
However, one of the tests at the top level fails, so the CI will still fail even after the formatting issues are fixed.

hswopeams · 2025-09-08T10:16:42Z

There are a lot of public functions that should be external. I will clean that up in a separate PR after these all get merged.

hswopeams · 2025-09-08T10:19:41Z

I noticed there aren't any changes to the Chainweb.t.sol file. This is the reason one of the tests if failing. Usually making code changes should means adding or modifying test cases.

javadkh2 · 2025-09-08T12:46:40Z

+     * script that utilizes this contract. It is safe to call this functions on
+     * an uninitialized chain.
+     */
+    function switchChain(uint256 chainId) public {


I don't think we should mix switchChain with setup chains.
Because we use different ways for seting up chains in test and scripts mode. that was why I have created two setup functions.
Also deployChainWebChainIdContract and deploySha512_256Precompile should happen only for test because the script can run agains normal devnet not only anvil.
I will move fork creation to the setup function instead that I think can fix the nonce issue

The chainweb chainid contract is deployed only in the tests. The function includes a check that deploys it only if it is not already present. Including this check seems the most robust approach to me.

deploySha512_256Precompile is tricky, because it is a precompile. Even if it is present on the forked chain, it wouldn't be available on the fork itself and still needs to be mocked.

However, as mentioned in the PR description, this PR is still a draft, and I intend to move the SHA512 precompile to a separate PR. So, anything related to the SHA512/256 will be removed from this PR (if we decide to move forward with it at all).

hswopeams

See comments for requested changes.

hswopeams · 2025-09-08T11:04:34Z

+address constant SHA512_256_ADDRESS = 0x0000000000000000000000000000000000000420;
+
+
+contract Sha512_256Precompile {


This contract name is not compliant with solidity standards. It should be Sha512256Precompile;

Sha512256Precompile is as name obfuscates that 512 and 256 are separate numbers. Would something like Sha512t256Precompile be acceptable?

Generally, as pointed out in the PR message, the SHA512_256 precompile addition is still in Draft mode and will be moved to a separate PR. The code is not yet final and subject to change.

I will incorporate any comments from this review in the follow up PR.

hswopeams · 2025-09-08T13:51:19Z

+        assembly {
+            let success := 0x00
+            let memPtr := 0x20
+            let ptr := add(memPtr, 0x60)


In assembly, you need to be careful with memory management. It's easy to exceed or overwrite memory. You should check the max offset. If it exceeds the current free memory pointer (0x40), you should update it. Something like this after setting ptr:

// Calculate paddedLen: rounds up calldatasize() to the next multiple of 32 bytes (EVM word size). // This ensures memory operations are word-aligned and safe for copying/returning data. let paddedLen := shl(5, shr(5, add(calldatasize(), 31))) // Calculates the highest memory address the assembly code will use for writing or copying data. let maxOffset := add(ptr, add(0x44, paddedLen)) let freePtr := mload(0x40) if gt(maxOffset, freePtr) { mstore(0x40, maxOffset) }

yeah, the assembly needs more work. It may actually go away completely, because it does not seem to fix the issues during foundry simulation runs that I had hoped it would resolve. If it turns out that I can't get it to work without --skip-simulation we may as well implement the contract in plain solidity, which works fine in the test and dry-run environments.

hswopeams · 2025-09-08T13:59:34Z

+    fallback(bytes calldata) external returns (bytes memory) {
+        assembly {
+            let success := 0x00
+            let memPtr := 0x20


Avoid using magic numbers and use constants for known values as well. See some examples below.

let MEMORY_START := 0x20 // Solidity memory starts at 0x20
let HEADER_SIZE := 0x60 // Size reserved for header, which this appears to be
let DATA_OFFSET := 0x44 // Offset for calldata after header

let memPtr := MEMORY_START
let ptr := add(memPtr, HEADER_SIZE)

hswopeams · 2025-09-08T14:29:13Z

+            }
+
+            // implicitely abi decode the return value
+            return(add(memPtr, 0x40), 0x20)


Are you sure the precompile will always return 96 bytes of data? If not, the buffer could contain data from previous calls. The pointer should be initialized before the delegate call on line 43 like this:

mstore(memPtr, 0x00) // Zero out first 32 bytes of output buffer mstore(add(memPtr, 0x20), 0x00) // Zero out second 32 bytes of output buffer mstore(add(memPtr, 0x40), 0x00) // Zero out third 32 bytes of output buffer

That's a good point. But, yes, int this case it always returns the exact same amount of data.

hswopeams · 2025-09-08T14:36:47Z

+        }
+    }
+
+    function sha512_256(bytes memory input) public pure returns (bytes memory result) {


This function name is not compliant with solidity naming standards. It should be sha512256. The function name cannot have an underscore in the name.

Why is this function returning bytes when the library returns bytes32? Hashes are usually bytes32 in solidity.

Yeah, should be bytes32 now. Originally, this was to simulate the return type of the recompile, but it's not use like this any more.

hswopeams · 2025-09-08T14:59:47Z

+        }
+    }
+
+    function sha384(bytes memory message) internal pure returns (bytes32, bytes16) {


Consider some input checks just to make sure people don't cause themselves problems when testing:
require(message.length <= 4096, "Input too large: may exceed block gas limit");

This would allow 4 KB of input. This could be added to sha512 and sha512_256 as well.

hswopeams · 2025-09-08T15:02:54Z

+        );
+    }
+
+    function padMessage(bytes memory message) internal pure returns (bytes memory) {


It would be good to comment on why this particular padding functionality is used. I assume it mimics sha-2.

yes, this is part of the SHA2 implementation. This code is from the original implementation.

I think, it would be better to not copy-and-past the Sha2Ext code, but use it as a dependency.

hswopeams · 2025-09-08T15:22:54Z

-            chainIds[i] = i + _chainwebChainIdOffset;
-        }
-        return chainIds;
+    function deploySha512_256Precompile() internal {


This function name does not conform to solidity standards. It should be deploySha512256Precompile. The name cannot contain and underscore.

Co-authored-by: Heather <42716568+hswopeams@users.noreply.github.com>

larskuhtz added 4 commits September 3, 2025 11:20

Fix ChainwebChainId deployment during setupChainsForTests

2227b2a

Make Chainweb and Chainweb ConfigRead contracts persistent

9c93f22

Add SHA512/256 Precompile

7a415ce

refactor initialization code to avoid selectFork in constructors

7bb5a73

larskuhtz marked this pull request as draft September 8, 2025 06:00

larskuhtz requested review from javadkh2 and jermaine150 September 8, 2025 06:02

larskuhtz mentioned this pull request Sep 8, 2025

Fix ChainwebChainId deployment during setupChainsForTests #12

Closed

hswopeams self-requested a review September 8, 2025 10:19

javadkh2 suggested changes Sep 8, 2025

View reviewed changes

hswopeams suggested changes Sep 8, 2025

View reviewed changes

larskuhtz and others added 2 commits September 8, 2025 15:44

Apply suggestion from @hswopeams

2c22674

Co-authored-by: Heather <42716568+hswopeams@users.noreply.github.com>

Apply suggestion from @hswopeams

72b8eac

Co-authored-by: Heather <42716568+hswopeams@users.noreply.github.com>

		address constant SHA512_256_ADDRESS = 0x0000000000000000000000000000000000000420;


		contract Sha512_256Precompile {

Uh oh!

Conversation

larskuhtz commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hswopeams commented Sep 8, 2025

Uh oh!

hswopeams commented Sep 8, 2025

Uh oh!

hswopeams commented Sep 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hswopeams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larskuhtz Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

larskuhtz commented Sep 8, 2025 •

edited

Loading

larskuhtz Sep 8, 2025 •

edited

Loading