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

Update tgrade contracts to v0.3.0-rc2 #85

Merged
merged 13 commits into from
Aug 12, 2021
Merged

Conversation

ethanfrey
Copy link
Contributor

@ethanfrey ethanfrey commented Aug 11, 2021

Closes #84

TODO:

  • Adds the metadata fields when registering validators.
  • Add message to update validator metadata
  • Expose metadata queries
  • Expose unbonding time queries

Not sure how to test all these, as there are no clear keeper tests I can model on.

I would ask to merge this and the rest can be built out in #87 (as it seems many tests are system level integration tests that need cli tools)

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Good stuff. Only minor comments.

To make use of update validator, we would need a CLI command in poe/client/cli

cmd/tgrade/testnet.go Outdated Show resolved Hide resolved
@@ -37,7 +39,21 @@ func RegisterValidator(ctx sdk.Context, contractAddr sdk.AccAddress, pk cryptoty
}

_, err = k.Execute(ctx, contractAddr, delegatorAddress, payloadBz, nil)
return sdkerrors.Wrap(err, "execute contract")
return sdkerrors.Wrap(err, "execute contract: register validator")
Copy link
Contributor

Choose a reason for hiding this comment

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

better error message 👍

@@ -128,9 +129,11 @@ func asJson(t *testing.T, m interface{}) string {
// TG4ValsetExecute Valset contract validator key registration
// See https://github.com/confio/tgrade-contracts/blob/main/contracts/tgrade-valset/schema/execute_msg.json
type TG4ValsetExecute struct {
RegisterValidatorKey RegisterValidatorKey `json:"register_validator_key"`
RegisterValidatorKey *RegisterValidatorKey `json:"register_validator_key,omitempty"`
UpdateMetadata *stakingtypes.Description `json:"update_metadata,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

pp: I had avoided to use sdk types mostly as they could change by a version upgrade. It is not ideal to manually maintain these object types here but we need to ensure stability in some way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had made a unique type, then realized it was the same, so why.

I can make a custom type that matches the contracts (not protobuf) and add a conversion function.

Copy link
Contributor Author

@ethanfrey ethanfrey Aug 11, 2021

Choose a reason for hiding this comment

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

Done in 0a21e3f

x/poe/keeper/msg_server.go Show resolved Hide resolved
x/poe/keeper/msg_server.go Outdated Show resolved Hide resolved
x/poe/types/msg.go Outdated Show resolved Hide resolved
@alpe alpe marked this pull request as ready for review August 11, 2021 12:46
@alpe alpe marked this pull request as draft August 11, 2021 12:46
@ethanfrey ethanfrey marked this pull request as ready for review August 11, 2021 19:31
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Very nice to see the contract integration.
The new queries look useful but they are not fully integrated into the stack without adding them to GRPC

Some improvements would be:

  • Cover the UpdateValidator functionality with tests
  • Add CLI command for update (edit) validator
  • Support new queries via GRPC endpoint
  • Add CLI support for new queries
  • Test new queries (end to end) via system tests

x/poe/contract/query.go Show resolved Hide resolved
@@ -1 +1 @@
v0.3.0-rc1
v0.3.0-rc2
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return response.UnbondingPeriod, err
}

func doQuery(ctx sdk.Context, k *twasmkeeper.Keeper, contractAddr sdk.AccAddress, query interface{}, result interface{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

good helper function 👍

x/poe/contract/query.go Outdated Show resolved Hide resolved
x/poe/contract/query.go Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Contributor Author

Some improvements would be:

Cover the UpdateValidator functionality with tests
Add CLI command for update (edit) validator
Support new queries via GRPC endpoint
Add CLI support for new queries
Test new queries (end to end) via system tests

I agree, but I got stuck as your testing is so much different than wasmd that I didn't know where to start. I wanted to make some keeper tests and do the queries, but couldn't figure out how to do so. Last time I tried to test something on tgrade I spent 3 hours porting over helpers from wasmd and then you said it was the wrong approach.

I would ask to merge this in with the infrastructure (and maybe adding the queries to grpc?) and I would ask you to add the cli and tgrade system tests. Maybe you can teach me how this work one day

@ethanfrey
Copy link
Contributor Author

Okay, made the requested adjustments. The missing "features" like exposing queries, cli, and system tests I will leave for another PR. I feel these belong to #87 (which should have an extended scope possibly)

@ethanfrey ethanfrey merged commit fb716f9 into main Aug 12, 2021
@ethanfrey ethanfrey deleted the 84-update-tgrade-contracts branch August 12, 2021 10:49
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.

Update to tgrade-contracts 0.3.0
2 participants