Metadata with confirmation #230
Conversation
Co-authored-by: adlrocha <6717133+adlrocha@users.noreply.github.com>
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
cryptoAtwill
force-pushed
the
store-validator-changes
branch
from
47a033a
to
0982ab4
Compare
binding/src/gateway_diamond.rs
Outdated
| @@ -211,6 +211,7 @@ pub mod gateway_diamond { | |||
| pub static GATEWAYDIAMOND_ABI: ::ethers::contract::Lazy<::ethers::core::abi::Abi> = | |||
| ::ethers::contract::Lazy::new(__abi); | |||
| #[rustfmt::skip] | |||
| <<<<<<< HEAD | |||
binding/src/subnet_actor_diamond.rs
Outdated
| @@ -227,11 +227,15 @@ pub mod subnet_actor_diamond { | |||
| pub static SUBNETACTORDIAMOND_ABI: ::ethers::contract::Lazy<::ethers::core::abi::Abi> = | |||
| ::ethers::contract::Lazy::new(__abi); | |||
| #[rustfmt::skip] | |||
| <<<<<<< HEAD | |||
src/SubnetActorDiamond.sol
Outdated
| @@ -56,8 +57,13 @@ contract SubnetActorDiamond { | |||
| s.majorityPercentage = params.majorityPercentage; | |||
| s.currentSubnetHash = s.parentId.createSubnetId(address(this)).toHash(); | |||
|
|
|||
| // We hardcode the current limit for active validators to 100 per Tendermint consensus | |||
| // We hardcode the current limit for active validators to 100 per Tendermint consensus | |||
| s.validatorSet.activeLimit = 100; | |||
There was a problem hiding this comment.
Could we make this into a constructor argument?
src/lib/LibStaking.sol
Outdated
| uint64 configurationNumber = recordChange({ | ||
| changes: changes, | ||
| validator: validator, | ||
| op: StakingOperation.Withdraw, |
There was a problem hiding this comment.
Is this Withdraw on purpose?
There was a problem hiding this comment.
it's a bug, I fixed it already.
|
|
||
| // confirm validators deposit immediately | ||
| LibStaking.depositWithConfirm(msg.sender, msg.value); | ||
| LibStaking.setMetadataWithConfirm(msg.sender, metadata); |
There was a problem hiding this comment.
This is where we discussed that we'll have to do https://github.com/consensus-shipyard/fendermint/issues/305 and that it will be as simple as checking that the address(metadata) == msg.sender.
cryptoAtwill
force-pushed
the
validator-metadata-change
branch
from
07cd874
to
5655676
Compare
|
@aakoshh The check of metadata is added, please take a look again. |
There was a problem hiding this comment.
LGTM. Just a few outstanding comments.
I see that this PR conflicts with a few of the things I wanted to address next on my PR #228, mainly:
- Remove the
f4for validators. - Have public key explicitly in the validator struct instead of as part of the metadata
I would suggest merging this one so I can merge this one and continue in #228?
|
|
||
| /// @notice Perform upsert operation to the withdraw changes, return total value to withdraw | ||
| /// @notice of the validator. | ||
| /// Each insert will increment the configuration number by 1, update will not. | ||
| function withdrawRequest(StakingChangeLog storage changes, address validator, uint256 amount) internal { | ||
| bytes memory payload = abi.encode(amount); |
There was a problem hiding this comment.
Why do you need to abi.encode the amount?
There was a problem hiding this comment.
As discussed sync, we are encoding all changes inside the payload to be more flexible with future changes. If we use this approach we need to have the encoding well-documented for users to be able to interpret what is happening easily.
| function _join(address validatorAddress) internal { | ||
| vm.prank(validatorAddress); | ||
| vm.deal(validatorAddress, DEFAULT_COLLATERAL_AMOUNT + 1); | ||
| // function _join(address validatorAddress) internal { |
There was a problem hiding this comment.
Should we remove this test or uncomment it?
There was a problem hiding this comment.
As discussed sync, we will keep them commented until we come back to writing a better test coverage for the gateway.
Co-authored-by: adlrocha <6717133+adlrocha@users.noreply.github.com>
…yard/ipc-solidity-actors into validator-metadata-change
| s.changeSet.nextConfigurationNumber = LibStaking.INITIAL_CONFIGURATION_NUMBER; | ||
| // The startConfiguration number is also 1 to match with nextConfigurationNumber, indicating we have | ||
| // empty validator change logs | ||
| s.changeSet.startConfigurationNumber = LibStaking.INITIAL_CONFIGURATION_NUMBER; |
There was a problem hiding this comment.
So startConfigurationNumber is the "first unapplied configuration"?
Update the metadata through confirmation.