Unable to fund state sync relayer when using rootchain originated native token #411

Stefan-Ethernal · 2023-10-24T08:37:50Z

This PR introduces addGenesisBalance function in CustomSupernetManager, which allows Supernets users who are using rootchain-originated token as native token to be able to make bridge transactions. It enables the genesis premine mechanism and is similar to what has been done for staking. Accounts that are about to be premined in the genesis time on the Supernets, need to lock their funds on root erc 20 predicate and then when Supernets is started from the genesis block, we are going to premine exactly the same amount of tokens, which prevents premining out of thin air.

coveralls · 2023-10-24T08:43:40Z

Pull Request Test Coverage Report for Build 6678652174

1 of 18 (5.56%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.7%) to 77.829%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
contracts/root/RootERC20Predicate.sol	1	2	50.0%
contracts/lib/GenesisLib.sol	0	4	0.0%
contracts/root/staking/CustomSupernetManager.sol	0	12	0.0%

Files with Coverage Reduction	New Missed Lines	%
contracts/lib/GenesisLib.sol	1	0.0%

Totals
Change from base Build 6312065521:	-0.7%
Covered Lines:	1285
Relevant Lines:	1595

💛 - Coveralls

Stefan-Ethernal · 2023-10-25T08:16:41Z

contracts/lib/GenesisLib.sol

@@ -13,8 +13,9 @@ enum GenesisStatus {
 }

 struct GenesisValidator {
-    address validator;
+    address addr;


@ZeroEkkusu Is this change safe to be made? The expectation for this PR is that the chain needs to be started from the genesis in order to be able to use the premine functionality however, I'm wondering what would happen for the already running chains when they apply this change and is it ok to rename it or revert it just in case to be backward compatible. WDYT?

Only in the sense that if someone's using the struct in their code, they'd need to update all .validator occurrences to .addr, but it doesn't change anything on the bytecode level.

The actual issue is that appending the uint256 balance shifts the storage layout. Maybe we could create a mapping somewhere instead and record it there.

See my following comment as well.

WDYT about such change? I've introduced an array which stores premine balances and exposed a function that retrieves it (because we are going to need it on the client-side, it is easier that way than having mapping and fetching balances by address key, one by one):
9f63d58

Re your comment/post at the end:

Do you mean that a restart will be required anyway? If so, different storage layouts don't matter because those would be fresh deployments with clean state.

If not:

This also shifts the storage layout of CustomSupernetManager (because there's GenesisSet private _genesis in it).

We can have the array defined and updated in the CustomSupernetManager directly (after all other storage variables and 1 subtracted from the gap), if that works for you.

Do you mean that a restart will be required anyway?

It will be required in case one uses rootchain originated token as a native token. Otherwise we don't expect chain reset (redeployment is not expected in that case).

In that case we'll need to align the storage layouts. Does the solution I proposed work for you?

We can have the array defined and updated in the CustomSupernetManager directly (after all other storage variables and 1 subtracted from the gap), if that works for you.

Yes, that should work. I'll change it that way most likely. 👍

ZeroEkkusu · 2023-10-26T10:41:02Z

contracts/lib/GenesisLib.sol

@@ -13,8 +13,9 @@ enum GenesisStatus {
 }

 struct GenesisValidator {
-    address validator;
+    address addr;


Only in the sense that if someone's using the struct in their code, they'd need to update all .validator occurrences to .addr, but it doesn't change anything on the bytecode level.

The actual issue is that appending the uint256 balance shifts the storage layout. Maybe we could create a mapping somewhere instead and record it there.

See my following comment as well.

contracts/root/staking/CustomSupernetManager.sol

contracts/root/RootERC20Predicate.sol

contracts/root/staking/CustomSupernetManager.sol

Stefan-Ethernal · 2023-10-26T20:37:09Z

The actual issue is that appending the uint256 balance shifts the storage layout. Maybe we could create a mapping somewhere instead and record it there.

If I'm not mistaken, GenesisSet is retrieved and read, only in bootstrapping time. With having premine function in place, I believe we don't have any other option but to recommend chain reset + regenesis (in order to keep the previous state). So my question is do we still have the issue with storage layouts being shifted, even though we are going to advise chain reset and contracts redeployment in this case (since we don't think there is a remediation to this issue, which would not require a chain reset). However still, if I have understood you correctly, this change would have a negative impact on the status field for example, which is accessed in the runtime (namely post-genesis), right?

I can introduce mapping(address => uint256) genesisBalances indeed if that is really required. I'd like to hear your opinion on this.

ZeroEkkusu

Re: Failing tests 96f761f

The tests failed because initialized checked nativeTokenRoot (the state variable) was not address(0), when it should have checked newNativeTokenRoot (the initialization parameter) instead.

Also, let's rename premine to something like addGenesisBalance, or similar.

ZeroEkkusu · 2023-10-30T12:50:07Z

contracts/lib/GenesisLib.sol

@@ -13,8 +13,9 @@ enum GenesisStatus {
 }

 struct GenesisValidator {
-    address validator;
+    address addr;


In that case we'll need to align the storage layouts. Does the solution I proposed work for you?

ZeroEkkusu · 2023-10-30T14:22:14Z

contracts/interfaces/root/staking/ICustomSupernetManager.sol

@@ -21,6 +21,7 @@ interface ICustomSupernetManager {
    event RemovedFromWhitelist(address indexed validator);
    event ValidatorRegistered(address indexed validator, uint256[4] blsKey);
    event ValidatorDeactivated(address indexed validator);
+    event AccountPremined(address indexed account, uint256 amount);


Update the event name.

Fixed and added the amount also as an indexed parameter 2d1dcc0

ZeroEkkusu · 2023-10-30T14:23:49Z

contracts/interfaces/root/staking/ICustomSupernetManager.sol

+
+    /// @notice getGenesisBalance returns balance for the given account address.
+    /// @param account address of the account
+    function getGenesisBalance(address account) external view returns (uint256);


Fixed 485e3c6

ZeroEkkusu · 2023-10-30T14:27:16Z

contracts/root/staking/CustomSupernetManager.sol

@@ -27,6 +28,8 @@ contract CustomSupernetManager is ICustomSupernetManager, Ownable2StepUpgradeabl

    GenesisSet private _genesis;
    mapping(address => Validator) public validators;
+    IRootERC20Predicate private _rootERC20Predicate;
+    mapping(address => uint256) private _genesisBalances;


We can expose the variable (make it public), and can remove the getter function below.

Fixed 485e3c6

ZeroEkkusu · 2023-10-30T14:27:52Z

contracts/root/staking/CustomSupernetManager.sol

+    }
+
+    /// @inheritdoc ICustomSupernetManager
+    function getGenesisBalance(address account) external view returns (uint256) {


Fixed 485e3c6

sonarcloud · 2023-10-31T05:48:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

Premine function implementation

366c197

Stefan-Ethernal force-pushed the EVM-865-unable-to-fund-relayer-when-using-rootchain-originated-native-token branch from ec746dd to 366c197 Compare October 24, 2023 09:58

Stefan-Ethernal added 2 commits October 24, 2023 12:08

Fix slither

bd06c5b

Unit tests

6baee8d

Stefan-Ethernal marked this pull request as ready for review October 25, 2023 08:12

Stefan-Ethernal commented Oct 25, 2023

View reviewed changes

Use correct forge-std revision

579029f

ZeroEkkusu requested changes Oct 26, 2023

View reviewed changes

Stefan-Ethernal added 2 commits October 26, 2023 22:09

Address comments (part 1)

96f761f

Remove blank line

ffc6b22

Stefan-Ethernal added 2 commits October 28, 2023 20:13

Pass tests again

5f491bf

Introduce genesisBalances field for storing premine balances

9f63d58

Stefan-Ethernal requested a review from ZeroEkkusu October 30, 2023 05:36

ZeroEkkusu requested changes Oct 30, 2023

View reviewed changes

Stefan-Ethernal added 3 commits October 30, 2023 14:22

Rename premine to addGenesisBalance

308282e

Remove nativeTokenRoot function and make field public

49f8f02

Track genesis balances in the CustomSupernetManager

32e63d4

Stefan-Ethernal requested a review from ZeroEkkusu October 30, 2023 13:51

ZeroEkkusu reviewed Oct 30, 2023

View reviewed changes

Stefan-Ethernal added 2 commits October 30, 2023 15:44

Rename event

2d1dcc0

Remove getGenesisBalance function

485e3c6

Stefan-Ethernal requested a review from ZeroEkkusu October 30, 2023 14:51

Minor fix

206a2e5

ZeroEkkusu approved these changes Oct 31, 2023

View reviewed changes

ZeroEkkusu merged commit 813c1b8 into 0xPolygon:dev Oct 31, 2023
8 of 9 checks passed

Stefan-Ethernal deleted the EVM-865-unable-to-fund-relayer-when-using-rootchain-originated-native-token branch October 31, 2023 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unable to fund state sync relayer when using rootchain originated native token #411

Unable to fund state sync relayer when using rootchain originated native token #411

Stefan-Ethernal commented Oct 24, 2023 •

edited

Loading

coveralls commented Oct 24, 2023 •

edited

Loading

Stefan-Ethernal Oct 25, 2023

ZeroEkkusu Oct 26, 2023

Stefan-Ethernal Oct 28, 2023

ZeroEkkusu Oct 30, 2023

Stefan-Ethernal Oct 30, 2023

ZeroEkkusu Oct 30, 2023

Stefan-Ethernal Oct 30, 2023

ZeroEkkusu Oct 26, 2023

Stefan-Ethernal commented Oct 26, 2023 •

edited

Loading

ZeroEkkusu left a comment

ZeroEkkusu Oct 30, 2023

ZeroEkkusu Oct 30, 2023

Stefan-Ethernal Oct 30, 2023 •

edited

Loading

ZeroEkkusu Oct 30, 2023 •

edited

Loading

Stefan-Ethernal Oct 30, 2023

ZeroEkkusu Oct 30, 2023

Stefan-Ethernal Oct 30, 2023

ZeroEkkusu Oct 30, 2023 •

edited

Loading

Stefan-Ethernal Oct 30, 2023

sonarcloud bot commented Oct 31, 2023

Unable to fund state sync relayer when using rootchain originated native token #411

Unable to fund state sync relayer when using rootchain originated native token #411

Conversation

Stefan-Ethernal commented Oct 24, 2023 • edited Loading

coveralls commented Oct 24, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6678652174

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stefan-Ethernal commented Oct 26, 2023 • edited Loading

ZeroEkkusu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stefan-Ethernal Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

ZeroEkkusu Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeroEkkusu Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 31, 2023

Stefan-Ethernal commented Oct 24, 2023 •

edited

Loading

coveralls commented Oct 24, 2023 •

edited

Loading

Stefan-Ethernal commented Oct 26, 2023 •

edited

Loading

Stefan-Ethernal Oct 30, 2023 •

edited

Loading

ZeroEkkusu Oct 30, 2023 •

edited

Loading

ZeroEkkusu Oct 30, 2023 •

edited

Loading