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

Implement pdao status and signalling address commands #599

Merged
merged 29 commits into from
Jul 24, 2024

Conversation

thomaspanf
Copy link
Member

@thomaspanf thomaspanf commented Jul 17, 2024

rocketpool pdao status
rocketpool pdao set-signalling-address
rocketpool pdao clear-signalling-address

Copy link

Coverage Report

Copy link
Contributor

@jshufro jshufro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is still in draft mode so I can't be too perturbed by the presence of commented out code, but good lord that's a lot of commented out code :)

rocketpool-cli/commands/pdao/status.go Outdated Show resolved Hide resolved
rocketpool-cli/commands/pdao/status.go Outdated Show resolved Hide resolved
rocketpool-cli/commands/pdao/status.go Outdated Show resolved Hide resolved
rocketpool-cli/commands/pdao/status.go Outdated Show resolved Hide resolved
rocketpool-cli/commands/pdao/status.go Outdated Show resolved Hide resolved
shared/types/api/pdao.go Outdated Show resolved Hide resolved
@@ -135,7 +135,8 @@ func (c *protocolDaoGetStatusContext) PrepareData(data *api.ProtocolDAOStatusRes

data.BlockNumber = uint32(c.blockNumber)
data.IsNodeRegistered = c.isNodeRegistered
// data.SignallingAddress = c.signallingAddress
data.SignallingAddress = c.signallingAddress
data.SignallingAddressFormatted = utils.GetFormattedAddress(c.ec, data.SignallingAddress)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably rename this to "GetPrimaryENS" or something.

We need to rethink our whole approach to ens, though, so it's a job for later: #583 (comment)

rocketpool-cli/commands/pdao/signalling-address.go Outdated Show resolved Hide resolved
rocketpool-daemon/api/pdao/clear-signalling-address.go Outdated Show resolved Hide resolved
Copy link

Coverage Report

Copy link

Coverage Report

Copy link

Coverage Report

rocketpool-cli/commands/pdao/status.go Outdated Show resolved Hide resolved
rocketpool-cli/commands/pdao/status.go Outdated Show resolved Hide resolved
rocketpool-daemon/api/pdao/status.go Outdated Show resolved Hide resolved
Copy link

Coverage Report

Remove const, fix message and nil pointer
@thomaspanf thomaspanf force-pushed the implement-pdao-status branch from 9b638f7 to 32a7c96 Compare July 18, 2024 22:20
Copy link

Coverage Report

Copy link

Coverage Report

@thomaspanf
Copy link
Member Author

#276d4ef

Snapshot isn't on holesky and I don't have a mainnet node so one of you guys should let me know if this works properly. That'd be much appreciated.

It should look something like this:

Rocket Pool has 1 Snapshot governance proposal(s) being voted on. You have voted on 0 of those. See details using 'rocketpool network dao-proposals'.

Copy link

Coverage Report

@thomaspanf
Copy link
Member Author

fee1e68

Should SanitizeHex() return an error if it isn't 0x prefixed? We should consider Joe's implementation, which doesn't require a 0x prefix and only removes it if present.

We can close NMC#17 if this looks good, since all the EIP712 related logic resides in the smartnode now.

@thomaspanf thomaspanf marked this pull request as ready for review July 19, 2024 06:34
Copy link
Contributor

@jshufro jshufro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the eip712 package is too fundamentally testable to not have a unit test. Please add one :)

shared/eip712/eip712.go Outdated Show resolved Hide resolved
shared/eip712/eip712.go Outdated Show resolved Hide resolved
shared/eip712/eip712.go Outdated Show resolved Hide resolved
shared/eip712/eip712.go Outdated Show resolved Hide resolved
rocketpool-daemon/api/pdao/set-signalling-address.go Outdated Show resolved Hide resolved
Copy link

Coverage Report

shared/eip712/eip712.go Outdated Show resolved Hide resolved
Copy link

Coverage Report

@jshufro
Copy link
Contributor

jshufro commented Jul 22, 2024

bleep bloop

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/rocket-pool/smartnode/v2/shared/eip712 94.44% (+94.44%) 🌟

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/rocket-pool/smartnode/v2/shared/eip712/eip712.go 94.44% (+94.44%) 18 (+18) 17 (+17) 1 (+1) 🌟

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • github.com/rocket-pool/smartnode/v2/shared/eip712/eip712_test.go

Copy link
Contributor

@jshufro jshufro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost there! the general shape is now correct

shared/eip712/eip712.go Outdated Show resolved Hide resolved
const EIP712Length = 65

// Pretty print for EIP712Components
func (e EIP712Components) String() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename this to Print(). Functions named String() should return a string to fulfill https://pkg.go.dev/fmt#Stringer

however, i think we want the Stringer interface to be implemented as:

func (e EIP712Components) String() string {
    out, err := e.MarshalText()
    if err != nil {
        // MarshalText should never return an error
        panic(err)
    }
    return string(out)
}

so that the "native" format is the 65 byte 0x-prefixed hex string.

shared/eip712/eip712.go Outdated Show resolved Hide resolved
shared/eip712/eip712_test.go Outdated Show resolved Hide resolved
Copy link

Coverage Report

Copy link

Coverage Report

Copy link

Coverage Report

shared/eip712/eip712.go Outdated Show resolved Hide resolved
shared/eip712/eip712.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jshufro jshufro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woops didn't mean to press approve yet

Copy link

Coverage Report

Copy link

Coverage Report

shared/eip712/eip712.go Show resolved Hide resolved
Copy link

Coverage Report

Copy link

Merging this branch changes the coverage (1 decrease, 1 increase)

Impacted Packages Coverage Δ 🤖
github.com/rocket-pool/smartnode/v2/client 0.00% (ø)
github.com/rocket-pool/smartnode/v2/rocketpool-cli/commands/node 0.06% (ø)
github.com/rocket-pool/smartnode/v2/rocketpool-cli/commands/pdao 2.62% (-0.14%) 👎
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/api/pdao 0.00% (ø)
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/common/contracts 0.00% (ø)
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/common/proposals 0.00% (ø)
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/common/services 0.00% (ø)
github.com/rocket-pool/smartnode/v2/shared/config 9.66% (ø)
github.com/rocket-pool/smartnode/v2/shared/eip712 86.84% (+86.84%) 🌟
github.com/rocket-pool/smartnode/v2/shared/types 0.00% (ø)
github.com/rocket-pool/smartnode/v2/shared/types/api 0.00% (ø)

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/rocket-pool/smartnode/v2/client/pdao.go 0.00% (ø) 47 (+3) 0 47 (+3)
github.com/rocket-pool/smartnode/v2/rocketpool-cli/commands/node/status.go 0.00% (ø) 185 0 185
github.com/rocket-pool/smartnode/v2/rocketpool-cli/commands/pdao/commands.go 32.56% (-2.88%) 86 (+7) 28 58 (+7) 👎
github.com/rocket-pool/smartnode/v2/rocketpool-cli/commands/pdao/signalling-address.go 0.00% (ø) 25 (+25) 0 25 (+25)
github.com/rocket-pool/smartnode/v2/rocketpool-cli/commands/pdao/status.go 0.00% (ø) 106 (+21) 0 106 (+21)
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/api/pdao/clear-signalling-address.go 0.00% (ø) 0 0 0
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/api/pdao/handler.go 0.00% (ø) 0 0 0
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/api/pdao/set-signalling-address.go 0.00% (ø) 0 0 0
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/api/pdao/status.go 0.00% (ø) 0 0 0
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/common/contracts/rocket-signer-registry.go 0.00% (ø) 0 0 0
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/common/proposals/proposal-manager.go 0.00% (ø) 0 0 0
github.com/rocket-pool/smartnode/v2/rocketpool-daemon/common/services/service-provider.go 0.00% (ø) 0 0 0
github.com/rocket-pool/smartnode/v2/shared/config/resources.go 38.46% (ø) 26 10 16
github.com/rocket-pool/smartnode/v2/shared/eip712/eip712.go 86.84% (+86.84%) 38 (+38) 33 (+33) 5 (+5) 🌟
github.com/rocket-pool/smartnode/v2/shared/types/api/pdao.go 0.00% (ø) 0 0 0
github.com/rocket-pool/smartnode/v2/shared/types/voting.go 0.00% (ø) 0 0 0

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • github.com/rocket-pool/smartnode/v2/shared/eip712/eip712_test.go

@0xfornax 0xfornax merged commit 5d7fedb into v2 Jul 24, 2024
5 checks passed
@0xfornax 0xfornax deleted the implement-pdao-status branch September 18, 2024 13:11
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.

3 participants