-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
…totype with tests
examples/solana/list-rewards/main.go
Outdated
) | ||
|
||
/* | ||
* Run the code with 'go run main.go' to view the rewards for Coinbase Cloud's public validator. |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
.github/workflows/lint.yaml
Outdated
push: | ||
branches: | ||
- master | ||
# We only run tests when we open PRs (and not for ex: on every commit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comment indenting
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, "", " ") |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe try the marshal options ? https://github.com/protocolbuffers/protobuf-go/blob/v1.33.0/encoding/protojson/encode.go#L45-L54
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice! Done 😄.
client/protocols/protocols.go
Outdated
package protocols | ||
|
||
import ( | ||
rewardspb "github.com/coinbase/staking-client-library-go/gen/go/coinbase/staking/rewards/v1" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
.Adds a filter builder for
ListStakes
based on the existing work forListRewards
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.