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

Release finding #585

Merged
merged 9 commits into from
Feb 2, 2023
Merged

Release finding #585

merged 9 commits into from
Feb 2, 2023

Conversation

kevin-yuhh
Copy link
Contributor

@kevin-yuhh kevin-yuhh commented Jan 19, 2023

Closes: #556
Related: #XXX

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]"

@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

❗ No coverage uploaded for pull request base (bounty@e4040d8). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             bounty     #585   +/-   ##
=========================================
  Coverage          ?   41.11%           
=========================================
  Files             ?      146           
  Lines             ?    13140           
  Branches          ?        0           
=========================================
  Hits              ?     5403           
  Misses            ?     7231           
  Partials          ?      506           


func NewReleaseFindingCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "release-finding",
Copy link
Contributor

Choose a reason for hiding this comment

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

Use release-finding 1 instead of release-finding --finding-id 1 , it may be more user-friendly

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 think all commands should use the same command format

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there are currently 2 formats, and should be unified.

@@ -3,6 +3,8 @@ package cli
import (
"bytes"
"crypto/rand"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/shentufoundation/shentu/v2/x/bounty/types"
Copy link
Contributor

Choose a reason for hiding this comment

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

should be sorted later

option (gogoproto.equal) = false;

uint64 finding_id = 1 [(gogoproto.moretags) = "yaml:\"finding_id\""];
Copy link
Contributor

Choose a reason for hiding this comment

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

We might also need release all or some findings of a given program.

Comment on lines 48 to 53
google.protobuf.Any encrypted_poc = 7 [(cosmos_proto.accepts_interface) = "EncryptedPoc",(gogoproto.moretags) = "yaml:\"encrypted_poc\""];
string poc = 8 [(gogoproto.moretags) = "yaml:\"poc\""];
string submitter_address = 9 [(gogoproto.moretags) = "yaml:\"submitter_address\""];
FindingStatus finding_status = 10 [(gogoproto.moretags) = "yaml:\"finding_status\""];
google.protobuf.Any encrypted_comment = 11 [(cosmos_proto.accepts_interface) = "EncryptedComment",(gogoproto.moretags) = "yaml:\"encrypted_comment\""];
string comment = 12 [(cosmos_proto.accepts_interface) = "Comment",(gogoproto.moretags) = "yaml:\"comment\""];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think encrypted_poc & poc or encrypted_comment & comment pairs can be merged to accept an interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I will merge these.

Comment on lines 78 to 79
limit := uint64(100)
page := uint64(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should accept these as flags

Comment on lines 96 to 101
res, err := queryClient.Findings(
cmd.Context(),
&types.QueryFindingsRequest{
ProgramId: programID,
Pagination: pageReq,
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably should just return this result

@@ -359,3 +358,107 @@ func HostProcessFinding(cmd *cobra.Command, args []string) (fid uint64,

return fid, commentAny, hostAddr, nil
}

func NewReleaseFindingCmd() *cobra.Command {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was deprecated in favor of providing the decrypted plaintext for each finding to be verified by encrypting it again and matching it with the on-chain encrypted ciphertext

Copy link
Collaborator

Choose a reason for hiding this comment

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

so you'd only be able to release findings one by 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.

make sense

Copy link
Contributor

@zheng-bin zheng-bin left a comment

Choose a reason for hiding this comment

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

Bounty's cli request method should be unified. Currently, there are two methods.

@kevin-yuhh kevin-yuhh merged commit 89bcf97 into bounty Feb 2, 2023
@kevin-yuhh kevin-yuhh deleted the release-finding branch February 2, 2023 02:15
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.

None yet

4 participants