feat(node): support configurable staking collateral token #1130

cryptoAtwill · 2024-09-06T07:00:29Z

This PR makes an effort to support configuration different token for collateral. Previously only native token is allowed, now ERC20 can be used, see #1024.

This PR introduces the following changes:

Rename SupplySource to GenericToken, SupplySourceHelper to GenericTokenHelper.
Introduced collateralSource parameter, this is where the token for collateral can be configured.
Aligned preFund, preRelease and everything related to token to use GenericToken, removed all hardcoding of native tokens, including LibStaking and ReleaseQueue.

List of todos:

To test in calibration network, try the following ipc configuration:

gateway_addr = "0xA4dDECD395590765C8623F9B9c49160080046d3c"
registry_addr = "0x06c89f2Aa8b580a66F273053B23d0622e6f87a14"

Or you can deploy your own ipc-stack.

Deploy your own ERC20 token on calibration, obtain the address. Then build the ipc-cli, run:

# create the subnet with collateral source
./target/release/ipc-cli subnet create --parent /r314159 --min-validator-stake 0.01 --min-cross-msg-fee 0 --min-validators 0 --bottomup-check-period 50 --permission-mode collateral --supply-source-kind native --collateral-source-kind erc20 --collateral-source-address <YOUR ERC20 ADDRESS>

# approve ERC20 token to spend 1 fil by the SubnetActor

# perform collateral related operations
./target/release/ipc-cli subnet join --subnet /r314159/${SUBNET} --collateral 0.02
./target/release/ipc-cli subnet stake --subnet /r314159/${SUBNET} --collateral 0.02

Co-authored-by: raulk <raul@protocol.ai>

…lidator-gating

…/ipc into validator-gating

JulissaDantes · 2024-09-20T11:29:21Z

contracts/contracts/subnet/SubnetActorManagerFacet.sol

            LibSubnetActor.enforceCollateralValidation();
        }
-        if (msg.value == 0) {
+        if (amount == 0) {


if the supplySource is native user is still obligated to send the msg.value number in the amount? is there a piece of logic somewhere that auto populates this field is the supplysource is native?

yep, have to do that. This just standardises the interface. So maybe one day nft can be used as well.

JulissaDantes · 2024-09-20T12:16:13Z

contracts/contracts/lib/GenericTokenHelper.sol

@@ -83,12 +88,12 @@ library SupplySourceHelper {
    /// triggered from the execution.
    /// This function the `safeTransfer` function used before.


Could you clarify this sentence?

JulissaDantes · 2024-09-20T12:22:17Z

contracts/contracts/subnet/SubnetActorManagerFacet.sol

-            return;
+            s.collateralSource.transferFunds(payable(msg.sender), amount);
+        } else {
+            LibStaking.withdraw(msg.sender, amount);


This else statement should also contain s.collateralSource.transferFunds(payable(msg.sender), amount); no? I ran a test that unstakes when the subnet is bootstrapped and the validator didnt get their erc20 back

so after the subnet is bootstrapped, that means any validator stake withdraw should not have occurred immediately because of safety issues. It has to go through the checkpointing and confirmed by the current commitee.

@JulissaDantes check this out:

ipc/contracts/contracts/lib/LibStaking.sol

Line 667 in 6d2cc3a

self.validators.confirmWithdraw(validator, amount);

It's buffered in a release queue.

See tests:

ipc/contracts/test/integration/SubnetActorDiamond.t.sol

Line 1902 in 6d2cc3a

saDiamond.rewarder().claim();

To withdraw stakes, a lot of checks have to go through.

oh, I think I'm little lost here,

what is the current commitee.?

does that mean there is somewhere else in the code where it gets check if any tokens are owed to a validator?

or does that mean that at the checkpoint library this is handled where they check unstake request and perform the token transfers?

And Im not clear here, my understanding is that a bootstrapped subnet is an active subnet that has the minimum required collateral and validators amount to run. So I still think that there is a path for a validator to just want to unstake certain amount of their stake after a while, where there are several checkpoints being made, and they just wish to reduce their staked amount, although if this is handled elsewhere I understand why its not the case, depending on the answers to the previous questions I guess.

Current committee refers to the current set of validators.

The main issue is not about token owed by the validator, this can be checked pretty easily. It's just that validator withdraw stake needs to be handled more carefully. I could be a malicious validator and I proposed a double spend or some malicious act. Then I immediately withdraw my collateral. If the withdraw go through immediately, then no punishment can be done. So this is just part of the protection mechanism. The current validators must "ack" this change through checkpointing. At the same time, the release queue holds the funds for a "challenge" period in case someone reports with misbehaviour. Keep things accountable.

AHH, thank you, Ive forgotten about the locking period, of course!

karlem

In general it looks good.

I just think that we do not have any tests with the collateral supply ERC20 source right?

contracts/contracts/lib/AssetHelper.sol

contracts/contracts/gateway/GatewayManagerFacet.sol

contracts/contracts/lib/AssetHelper.sol

ipc/cli/src/commands/subnet/create.rs

ipc/provider/src/manager/evm/manager.rs

karlem · 2024-09-20T14:33:31Z

ipc/provider/src/manager/evm/manager.rs

        }
    }

+    pub async fn handle_txn_token<B, D, M>(


I think I am missing the point of this function. Could you please explain?

because different combination of collateral source and supply source will have different txn value, if that's the question?

Alright I get it now. It is a bit unclear at first sight. Maybe comment would do.

comment added!

cryptoAtwill · 2024-09-20T15:21:01Z

@karlem what tests are you referring to? There are plenty of unit and integration tests for the contracts.

Co-authored-by: Karel Moravec <moravec.wdd@gmail.com>

…ard/ipc into collateral-sourcing

karlem · 2024-09-23T09:49:03Z

@karlem what tests are you referring to? There are plenty of unit and integration tests for the contracts.

@cryptoAtwill but they all use the native collateral source not the ERC20 right? Correct me if I'm wrong.

cryptoAtwill · 2024-09-23T09:51:22Z

@karlem not really, there are permutation and combination, say:

ipc/contracts/test/integration/SubnetActorDiamond.t.sol

Line 1942 in 012fa69

    
           function testSubnetActorDiamond_CollateralERC20_SupplyERC20_SameToken_RegisteredInGateway() public {

raulk

Will submit a PR to:

Enclose the allowance-related complexity inside the AssetHelper.
Remove the unnecessary error types.
Use SafeERC20#safeIncreaseAllowance() instead of implementing this logic ourselves.

This approval is conditional to reviewing and merging the upcoming PR.

raulk · 2024-09-23T15:03:55Z

contracts/contracts/gateway/GatewayManagerFacet.sol

+        if (genesisCircSupply > 0) {
+            SubnetActorGetterFacet(msg.sender).supplySource().lock(genesisCircSupply);
+        }
+        if (collateral > 0) {
+            SubnetActorGetterFacet(msg.sender).collateralSource().lock(collateral);


I would support dropping this feature entirely: #1142

raulk · 2024-09-23T15:10:18Z

contracts/contracts/errors/IPCErrors.sol

 error NotEnoughGenesisValidators();
 error ValidatorPowerChangeDenied();
+error IncreaseAllowanceFailed();
+error Unreachable();


This error type is not needed as it doesn't provide more information than a revert with a string message.

raulk · 2024-09-23T15:33:59Z

contracts/contracts/lib/AssetHelper.sol

+            // now we are handling native token
+            if (msg.value < value) {
+                revert NoBalanceIncrease();
+            }


There is no "handling" here, just an extra assertion.

There is no "native token", but "native coins".

This can be an else if.

Why is this not an equals check?

if msg.value == value, we should pass the check. If msg.value > value, someone wants to do charity, maybe can just take it... or maybe not, haha. I will change this to ==.

Best not to. It's stronger to have an invariant that gateway balance across all tokens and the native coin is zero for initialized subnets, than to have one where it can be non zero.

Oh, I realized we have to use msg.value > value. The reason is caller might have msg.value more than just locked value, for other purposes as well. In that way, we can only enforce msg.value > value.

raulk · 2024-09-23T16:00:28Z

contracts/contracts/lib/LibStaking.sol

+                        IGateway(gateway).addStake(amount);
+                    }
+                } else {
+                    revert Unreachable();


Just an unqualified revert with an error string is fine.

raulk · 2024-09-23T16:23:18Z

contracts/contracts/lib/LibStaking.sol

+                    if (s.collateralSource.isNative()) {
+                        IGateway(gateway).addStake{value: amount}(amount);
+                    } else {
+                        s.collateralSource.increaseAllowance(gateway, amount);
+                        IGateway(gateway).addStake(amount);
+                    }


This is rather leaky. The AssetHelper is designed to hide the different asset kinds. I would refactor this as a new function AssetHelper#makeSpendable that makes an amount spendable by some party.

Note: even if you want to keep this conditional here, you don't need an isNative helper, you could just access the kind field of the Asset.

raulk · 2024-09-24T20:02:23Z

contracts/contracts/lib/AssetHelper.sol

+        if (self.kind == AssetKind.ERC20) {
+            IERC20 token = IERC20(self.tokenAddress);
+            uint256 allowance = token.allowance(address(this), spender);
+            if (!token.approve(spender, allowance + amount)) {
+                revert IncreaseAllowanceFailed();
+            }
+        }


Just use SafeERC20#safeIncreaseAllowance()?

Singleton ifs like this are cleaner if written as a negative if with an early return.

raulk · 2024-09-24T20:05:32Z

contracts/contracts/lib/LibSubnetActor.sol

                emit SubnetBootstrapped(s.genesisValidators);

                // register adding the genesis circulating supply (if it exists)
-                IGateway(s.ipcGatewayAddr).register{value: totalCollateral + s.genesisCircSupply}(s.genesisCircSupply);


Looks like we had a bug here, where we sent native coins representing the genesis circulating supply, even if the supply source was ERC20. Nice catch!

raulk · 2024-09-24T20:09:39Z

contracts/contracts/lib/LibSubnetActor.sol

+        if (!supplySourceNative) {
+            s.supplySource.increaseAllowance(s.ipcGatewayAddr, genesisCircSupply);
+        } else {
+            msgValue += genesisCircSupply;
+        }
+
+        if (!collateralSourceNative) {
+            s.collateralSource.increaseAllowance(s.ipcGatewayAddr, collateral);
+        } else {
+            msgValue += collateral;
+        }


This is leaky as mentioned elsewhere. I'm going to take a stab to contain this complexity inside the AssetHelper.

raulk

Will submit a PR to:

Enclose the allowance-related complexity inside the AssetHelper.
Remove the unnecessary error types.
Use SafeERC20#safeIncreaseAllowance() instead of implementing this logic ourselves.

This approval is conditional to reviewing and merging the upcoming PR.

raulk · 2024-09-24T20:25:16Z

@cryptoAtwill all blocking review comments addressed in: #1147

Please review and merge if you agree, then feel free to merge this one.

@karlem when that's done, could you please make a release?

The general policy should be that if we're not expecting users to catch these errors to handle them specifically, nor are we seeking to optimise gas and/or return size (at the expense of debuggability), we shouldn't be creating dedicated error types, let alone extremely narrow and tiny ones.

cryptoAtwill and others added 16 commits August 29, 2024 16:59

initial gating

49555f7

collateral validator gating

5697ee4

Update contracts/contracts/lib/LibSubnetActor.sol

2e7761c

Co-authored-by: raulk <raul@protocol.ai>

Update contracts/contracts/lib/LibSubnetActor.sol

43ee469

Co-authored-by: raulk <raul@protocol.ai>

review feedbacks

d961819

Merge branch 'main' of protocol-github:consensus-shipyard/ipc into va…

05d1095

…lidator-gating

adding tests

9f00973

update gater interface

7102a78

update ipc

92a1dc4

Merge branch 'main' into validator-gating

2cec1a7

update test

374a7df

Merge branch 'validator-gating' of protocol-github:consensus-shipyard…

6057dd8

…/ipc into validator-gating

adding deployments and sample cli

e89c069

lint

4c6000c

wip

0bfa4ec

adding tests

cccf84e

cryptoAtwill requested a review from a team as a code owner September 6, 2024 07:00

cryptoAtwill added 5 commits September 9, 2024 15:06

ipc-cli update

19d6158

fix ci

07f7182

fix linked token error

80db07b

fix ci

4743a12

increase allowance instead of approve

4fc0e73

raulk linked an issue Sep 17, 2024 that may be closed by this pull request

[Staking] Token-bound validator collateral #1024

Closed

karlem added 2 commits September 18, 2024 19:15

feat: add simple docs

3dcacd0

fix: typo

9388b24

Base automatically changed from validator-gating to main September 18, 2024 20:33

raulk added 2 commits September 19, 2024 16:37

Merge branch 'validator-gating' into collateral-sourcing

ee3a9a5

Merge branch 'main' into collateral-sourcing

6d2cc3a

JulissaDantes reviewed Sep 20, 2024

View reviewed changes

update comment

4c7e013

JulissaDantes reviewed Sep 20, 2024

View reviewed changes

cryptoAtwill added 3 commits September 20, 2024 20:36

rename generic token

8e6e1e8

merge with upstream

c997cba

fix rust naming

7f2cb84

karlem reviewed Sep 20, 2024

View reviewed changes

cryptoAtwill and others added 4 commits September 23, 2024 11:38

Update contracts/contracts/subnet/SubnetActorGetterFacet.sol

d91dc44

Co-authored-by: Karel Moravec <moravec.wdd@gmail.com>

finish renaming

5e7fc74

Merge branch 'collateral-sourcing' of protocol-github:consensus-shipy…

b868fd1

…ard/ipc into collateral-sourcing

fix fmt

012fa69

cryptoAtwill added 2 commits September 23, 2024 21:43

update review feedbacks

6626ee3

update comment

b3b3516

raulk mentioned this pull request Sep 23, 2024

Simplification: remove the ability to contribute collateral at subnet creation #1142

Closed

cryptoAtwill added 2 commits September 23, 2024 23:31

update comment

6a0e65c

fmt

7cc73c3

raulk approved these changes Sep 24, 2024

View reviewed changes

raulk and others added 3 commits September 25, 2024 09:33

feat(node): address review comments against #1130 (#1147)

ea5e69a

more comments

78443ea

improve comment.

71cae9f

raulk force-pushed the collateral-sourcing branch from 03994f1 to 71cae9f Compare September 25, 2024 09:12

raulk and others added 3 commits September 25, 2024 11:12

Merge branch 'main' into collateral-sourcing

c6820a4

fix error checker

bdbbc6a

cryptoAtwill merged commit b0b1dd0 into main Sep 25, 2024

cryptoAtwill deleted the collateral-sourcing branch September 25, 2024 10:45

		@@ -83,12 +88,12 @@ library SupplySourceHelper {
		/// triggered from the execution.
		/// This function the `safeTransfer` function used before.

feat(node): support configurable staking collateral token #1130

feat(node): support configurable staking collateral token #1130

Uh oh!

Conversation

cryptoAtwill commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

karlem left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

cryptoAtwill commented Sep 20, 2024

Uh oh!

karlem commented Sep 23, 2024

Uh oh!

cryptoAtwill commented Sep 23, 2024

Uh oh!

raulk 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!

cryptoAtwill Sep 25, 2024 • 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!

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!

cryptoAtwill commented Sep 6, 2024 •

edited

Loading

cryptoAtwill Sep 25, 2024 •

edited

Loading