New queries on open submission windows #896
Conversation
xmariachi
requested review from
amimart,
guilherme-brandao,
spooktheducks and
zale144
as code owners
|
The latest Buf updates on your PR. Results from workflow Buf Linter / buf (pull_request).
|
| return &types.GetReputerLossBundlesAtBlockResponse{LossBundles: reputerLossBundles}, nil | ||
| } | ||
|
|
||
| func (qs queryServer) GetOpenReputerSubmissionWindows(ctx context.Context, req *types.GetOpenReputerSubmissionWindowsRequest) (_ *types.GetOpenReputerSubmissionWindowsResponse, err error) { |
There was a problem hiding this comment.
this feels like a strange file (based on name) to add these to. normally i would call this a nitpick, but given that we're trying to provide a good foundation (not just in terms of architecture but also semantics, so to make the codebase more easily navigable) i think it's not just a nitpick
There was a problem hiding this comment.
agreed, changed to its own file
| s.Require().Equal(removalInfo.Amount, retrievedInfo.Amount) | ||
| } | ||
|
|
||
| func (s *QueryServerTestSuite) TestGetOpenReputerSubmissionWindows() { |
There was a problem hiding this comment.
i would write this as a table-driven test. would make it easy to add more cases. i can think of maybe 4-6 that would be useful -- and less code to do it this way.
it actually helps w/r/t clarity to do so. it's a more compact and readable definition of expectations. and can be statically analyzed more easily if we ever need to build more tools like that (we've got a head start on it via our custom linters)
| s.Require().Equal(int64(1000), response.Nonces.Nonces[0].ReputerNonce.BlockHeight, "Should be nonce1") | ||
| } | ||
|
|
||
| func (s *QueryServerTestSuite) TestGetOpenWorkerSubmissionWindows() { |
| s.Require().Equal(int64(1000), response.Nonces.Nonces[0].BlockHeight, "Should be nonce1") | ||
| } | ||
|
|
||
| func (s *QueryServerTestSuite) TestGetOpenReputerSubmissionWindowsWithMultipleNoncesInWindow() { |
There was a problem hiding this comment.
even more than the last 2, table will shine here
| ) | ||
|
|
||
| // Add multiple nonces that will be open at the same time | ||
| nonce1 := &types.Nonce{BlockHeight: 1000} // Open at 1100-1200 |
There was a problem hiding this comment.
most intervals in practice are half-closed
| } | ||
| } | ||
|
|
||
| func (s *KeeperTestSuite) TestGetOpenReputerSubmissionWindows() { |
| s.Require().Empty(openNonces.Nonces, "Should have no open nonces after window closes") | ||
| } | ||
|
|
||
| func (s *KeeperTestSuite) TestGetOpenWorkerSubmissionWindows() { |
| s.Require().Empty(openNonces.Nonces, "Should have no open nonces after window closes") | ||
| } | ||
|
|
||
| func (s *KeeperTestSuite) TestGetOpenReputerSubmissionWindowsWithMultipleNoncesInWindow() { |
x/emissions/keeper/keeper_test.go
Outdated
|
|
||
| _, err := k.GetOpenReputerSubmissionWindows(s.Ctx(), nonExistentTopicId) | ||
| s.Require().Error(err) | ||
| s.Require().Contains(err.Error(), "error getting topic") |
There was a problem hiding this comment.
we should start defaulting to using github.com/brynbellomy/go-utils/errors and then doing
if errors.Cause(err) == ErrSomePredefinedError
this is better than string matching.
the way you construct such an error is
var ErrSomePredefinedError = errors.New("predefined error")
errors.Wrap(ErrSomePredefinedError, "some custom extra message")
(or just return ErrSomePredefinedError)
this is how it's done in Go
There was a problem hiding this comment.
Yep - using types.ErrTopicDoesNotExist as we do in other parts.
An oversight from agentic code.
| {ProtoField: "topic_id"}, | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
would love to see the Core team submit PRs to the Python and Go SDKs when new query methods are added. the only people who use the cli outside of our internal team are occasionally validators
what is the connection with users for this feature? Python SDK, realistically
|
took another look at this, and it's still in need of table-driven tests, which are must easier/faster to make exhaustive (and easier to reason about because you're not looking at all of the lines of implementation code, but rather the values in the "table" itself) |
Purpose of Changes and their Description
Are these changes tested and documented?
emissionsmodule'sREADMEUnreleasedsection ofCHANGELOG.md?