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 Repo, Add List Stakes Filter #16

Merged
merged 16 commits into from
Apr 24, 2024
Merged

Update Repo, Add List Stakes Filter #16

merged 16 commits into from
Apr 24, 2024

Conversation

ProfMoo
Copy link
Contributor

@ProfMoo ProfMoo commented Apr 24, 2024

  1. Adds some developer experience niceties to improve the experience in this repo. Namely, adding some GitHub Action workflows for linting/testing and cleaning up some Makefile targets. Since we run linting on every PR now, the linting needed to be fixed. Many of the changes in this PR are from the linting fixes, which includes formatting fixes per the rules in .golangci-lint.yaml.

  2. Adds a filter builder for ListStakes based on the existing work for ListRewards

  3. Adds some existing changes that we had for the provided examples but hadn't added to coinbase/staking-client-library-go yet. Includes README updates to the examples.

)

/*
* Run the code with 'go run main.go' to view the rewards for Coinbase Cloud's public validator.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe give absolute path here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh good idea, I'll do that.

push:
branches:
- master
# We only run tests when we open PRs (and not for ex: on every commit)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: comment indenting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed.

rewardspb "github.com/coinbase/staking-client-library-go/gen/go/coinbase/staking/rewards/v1"
"github.com/coinbase/staking-client-library-go/client/protocols"
"github.com/coinbase/staking-client-library-go/client/rewards/reward"
api "github.com/coinbase/staking-client-library-go/gen/go/coinbase/staking/rewards/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we inconsistently import "github.com/coinbase/staking-client-library-go/gen/go/coinbase/staking/rewards/v1" with api, rewardspb or stakingpb. Would be nice to stick to one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh definitely. I'll standardize on api for now.

@@ -53,12 +63,11 @@ func main() {
log.Fatalf("error listing stakes: %s", err.Error())
}

d, err := protojson.Marshal(stake)
marshaled, err := json.MarshalIndent(stake, "", " ")
Copy link
Contributor

Choose a reason for hiding this comment

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

using json.MarshalIndent instead of protojson.Marshal might change the output in which case we should update the Readme.

Any reason to do this ? I think I used protojson.Marshal as it did a better job at marshaling enums (I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh you're totally right json.MarshalIndent doesn't correctly handle enums 😞. I moved away from protojson.Marshal because it didn't pretty print the results. I just updated it to a version where I think we can get the best of both words (with some extra lines), LMK what you think (or if you know of a more concise way to do this).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nice! Done 😄.

package protocols

import (
rewardspb "github.com/coinbase/staking-client-library-go/gen/go/coinbase/staking/rewards/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think considering today's situation - we should put this in the rewards pkg as its not something common to both our APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh I think that's a good callout. It'd be great to combine these client experiences over time, but will move these protocols to rewards for now.

@ProfMoo ProfMoo requested a review from drohit-cb April 24, 2024 19:41
@drohit-cb drohit-cb merged commit f50b22c into main Apr 24, 2024
4 checks passed
@drohit-cb drohit-cb deleted the list-stakes-filter branch April 24, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants