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

finding withdrawal #590

Merged
merged 7 commits into from
Feb 2, 2023
Merged

finding withdrawal #590

merged 7 commits into from
Feb 2, 2023

Conversation

0311xuyang
Copy link
Contributor

Closes: #553
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 27, 2023

Codecov Report

❗ No coverage uploaded for pull request base (bounty@c5d0ad0). 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     #590   +/-   ##
=========================================
  Coverage          ?   41.17%           
=========================================
  Files             ?      146           
  Lines             ?    13247           
  Branches          ?        0           
=========================================
  Hits              ?     5454           
  Misses            ?     7284           
  Partials          ?      509           

@0311xuyang 0311xuyang mentioned this pull request Jan 27, 2023
11 tasks
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeWithdrawalFinding,
sdk.NewAttribute(types.AttributeKeyFindingID, strconv.FormatUint(msg.FindingId, 10)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to add a programId event attribute?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes it should be better

message MsgHostRejectFindingResponse {}

message MsgWithdrawalFinding {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MsgCancelFinding should be better

@@ -34,6 +34,7 @@ func NewTxCmd() *cobra.Command {
NewSubmitFindingCmd(),
NewHostAcceptFindingCmd(),
NewHostRejectFindingCmd(),
NewWithdrawalFindingCmd(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

NewCancelFindingCmd

return k.SetPidFindingIDList(ctx, pid, fids)
}
}
return fmt.Errorf("finding id not exists")
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't exist or does not exist

store := ctx.KVStore(k.storeKey)
store.Delete(types.GetProgramIDFindingListKey(pid))
return nil

Copy link
Collaborator

Choose a reason for hiding this comment

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

delete empty line

ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeWithdrawalFinding,
sdk.NewAttribute(types.AttributeKeyFindingID, strconv.FormatUint(msg.FindingId, 10)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

yes it should be better


// Finding
const (
errFindingNotExists uint32 = iota + 201
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably better to merge these errors with above

TypeMsgSubmitFinding = "submit_finding"
TypeMsgAcceptFinding = "accept_finding"
TypeMsgRejectFinding = "reject_finding"
TypeMsgWithdrawalFinding = "withdrawal_finding"
Copy link
Collaborator

Choose a reason for hiding this comment

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

cancel_finding

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.

LGTM

@0311xuyang 0311xuyang merged commit e1a15f7 into bounty Feb 2, 2023
@0311xuyang 0311xuyang deleted the finding-delete branch February 2, 2023 04: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.

None yet

4 participants