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(Profile): allow change consensus address #236

Merged
merged 126 commits into from
Nov 27, 2023
Merged

Conversation

nxqbao
Copy link
Contributor

@nxqbao nxqbao commented May 29, 2023

Description

  • Support change address of validator, including admin, consensus, treasury
    • Treat the values of addresses in Profile as single of truth, while in others are shadow from these values.
    • Details of the affected contracts can be found in the NatSpec of the Profile contract.
  • Remove the related code of Bridge in the SlashIndicator
  • Enhance code consistency by updating _-prefix in many contracts.
  • [Hardhat Test] Improve test setup, breakdown fixture into fixture and initializers

Support features:

  • change admin address
  • change consensus address
  • change bridge operator address: removed due to [Staking | ValidatorSet] Feat: Bridge Logic Removal #253
  • Move all address query methods into the Profile contract
    • Query by the Profile contract
    • Preserve the existing queries on Validator and Staking contract
    • Query governor and bridge governor in Trusted Org contract

Testing

[Foundry Test] The test for this PR is maintained in #301

Contract changes

The table below shows the following info:

  • Logic: the logic is changed.
  • ABI: the ABI is changed.
  • Init data: new storage field is declared and needs initializing.
  • Dependent: needs to be changed due to changes in other contracts.
Contract name Logic ABI Init data Dependent
Profile x x x
Staking x x
ValidatorSet x x x
Maintenance x x
SlashIndicator x x
RoninTrustedOrg x x x

Checklist

  • I have clearly commented on all the main functions following the NatSpec Format
  • The box that allows repo maintainers to update this PR is checked
  • I tested locally to make sure this feature/fix works

@nxqbao nxqbao force-pushed the feat/profile branch 4 times, most recently from 2d08bbc to 6d44e9b Compare June 15, 2023 07:17
@nxqbao
Copy link
Contributor Author

nxqbao commented Jun 20, 2023

Need to consider should put whether the consensus addr or the validator id in the Slashed event, or both, due to non-validator will be treated as address(0) in slashing double signing.

  1) Slash indicator test
       Single flow test
         Double signing slash
           Should be able to slash non-validator with double signing:

      AssertionError: expected '0x00000000000000000000000000000000000…' to equal '0x4628B9a23BDe2402379A667388e3aF213A7…'
      + expected - actual

      -0x0000000000000000000000000000000000000000
      +0x4628B9a23BDe2402379A667388e3aF213A70e73d
      
      at async Context.<anonymous> (test/slash/SlashIndicator.test.ts:603:9)

@nxqbao nxqbao marked this pull request as ready for review November 7, 2023 12:52
@nxqbao nxqbao merged commit 1cafa6a into release/v0.7.0 Nov 27, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant