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

[Miner] chore: scaffold create-claim message #43

Merged
merged 6 commits into from
Oct 17, 2023

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Oct 10, 2023

Summary

This message type is a nested dependency of the Miner via the SupplierClient.

  • Scaffolded MsgCreateClaim message types, handlers, etc.:
    ignite scaffold message create-claim session_header:SessionHeader root_hash --signer SupplierAddress --module supplier
    
    (NB: generated a temporary SessionHeader duplicate in the supplier module so the above would work; subsequently, removed and updated protobuf import and usage.)
  • Switch MsgCreateClaim#root_hash type from string to []byte & refactor usages.

Issue

Related to:

This message type is a nested dependency of the Miner via the SupplierClient.

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make go_test
  • Verify Localnet manually: See the instructions [here](TODO: add link to instructions)

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@bryanchriswhite bryanchriswhite added the miner Changes related to the Miner label Oct 10, 2023
@bryanchriswhite bryanchriswhite added this to the Shannon TestNet milestone Oct 10, 2023
@bryanchriswhite bryanchriswhite self-assigned this Oct 10, 2023
@bryanchriswhite bryanchriswhite linked an issue Oct 10, 2023 that may be closed by this pull request
10 tasks
@bryanchriswhite bryanchriswhite force-pushed the chore/scaffold-claim-msg branch 2 times, most recently from acfdf18 to 7f7c544 Compare October 10, 2023 09:30
@bryanchriswhite bryanchriswhite marked this pull request as ready for review October 10, 2023 09:31
@bryanchriswhite bryanchriswhite added the supplier Changes related to the Supplier actor label Oct 10, 2023
proto/pocket/supplier/tx.proto Outdated Show resolved Hide resolved
proto/pocket/supplier/tx.proto Outdated Show resolved Hide resolved
@red-0ne
Copy link
Contributor

red-0ne commented Oct 10, 2023

ignite scaffold message create-claim SupplierAddress SmstRootHash SessionId SessionNumber:uint InvalidationHeight:int --module supplier

InvalidationHeight doesn't seem to be used

@bryanchriswhite
Copy link
Contributor Author

ignite scaffold message create-claim SupplierAddress SmstRootHash SessionId SessionNumber:uint InvalidationHeight:int --module supplier

InvalidationHeight doesn't seem to be used

Nice catch! 👍 I had already removed it from the code but forgot to update the scaffold command in the description.

@bryanchriswhite bryanchriswhite marked this pull request as draft October 11, 2023 10:40
`ignite scaffold message create-claim session_header:SessionHeader root_hash --signer SupplierAddress --module supplier`
(NB: generated a temporary SessionHeader duplicate in the supplier module so the above would work; subsequently, removed and updated protobuf import and usage.)
@bryanchriswhite bryanchriswhite force-pushed the chore/scaffold-claim-msg branch from 7f7c544 to d6ce316 Compare October 11, 2023 21:31
@bryanchriswhite bryanchriswhite changed the base branch from main to refactor/shared-supplier-type October 11, 2023 21:31
@bryanchriswhite bryanchriswhite marked this pull request as ready for review October 11, 2023 21:32
Olshansk
Olshansk previously approved these changes Oct 13, 2023
proto/pocket/supplier/tx.proto Show resolved Hide resolved
x/supplier/client/cli/tx_create_claim.go Show resolved Hide resolved
x/supplier/types/message_create_claim_test.go Show resolved Hide resolved
x/supplier/keeper/msg_server_create_claim.go Show resolved Hide resolved
Base automatically changed from refactor/shared-supplier-type to main October 13, 2023 06:02
@bryanchriswhite bryanchriswhite dismissed Olshansk’s stale review October 13, 2023 06:02

The base branch was changed.

Co-authored-by: Daniel Olshansky <[email protected]>
@bryanchriswhite bryanchriswhite force-pushed the chore/scaffold-claim-msg branch from a68e595 to 20c6373 Compare October 13, 2023 14:35
* main:
  [Gateway] Scaffold the Gateway type (#57)
  [Gateway] Scaffold Gateway module and nothing else
  fix: post-merge imports
  [Session] Adding the Session type (#54)
  [WIP][Session] Adding the Relay type (#53)
x/supplier/keeper/msg_server_create_claim.go Show resolved Hide resolved
x/supplier/keeper/msg_server_create_claim.go Show resolved Hide resolved
- supplier address
- session header
- claim
2. [ ] last block height commitment; derives:
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this more relevant to the proof flow?

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Oct 18, 2023

Choose a reason for hiding this comment

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

It is, but it must be committed to already at the point. IIRC, the point was to have the client commit to some block to say, "this is the latest block I'm aware of, so please use it's block hash" (or something similar; off-by-one, etc.). Then it's up to the on-chain logic to decide if that block is too old to be used as the pseudo-random input for the msg distribution logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Not to be confused with the pseudo-random input for the proof path.) 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@red-0ne 👆

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

LGTM.

@red-0ne I saw your comments but believe it's okay to merge it in without resolving them. I see them as guidelines for the reviewer to document and share what the next step is, but not anything set in stone.

I think it's a great example of why we need to keep these PRs small because we can potentially have a whole day of discussion on each of the bullet points, but need to keep things moving in smaller chunks.

@bryanchriswhite bryanchriswhite merged commit ab0a1f1 into main Oct 17, 2023
3 checks passed
bryanchriswhite added a commit that referenced this pull request Oct 17, 2023
* pokt/main:
  [Miner] chore: scaffold create-claim message (#43)
bryanchriswhite added a commit that referenced this pull request Oct 18, 2023
* pokt/main:
  [Service] Added the `Service` type
  Updated the command for unit tests in the PR template
  Updated PR template
  [Miner] chore: scaffold create-claim message (#43)
  [E2E] Add basics of the E2E suite (#62)
  [Gateway] Scaffold the unstake gateway message and nothing else (#67)
  [Gateway] Scaffold the gateway stake message and nothing else (#66)
  Add placeholder for auto-generated packages
@bryanchriswhite bryanchriswhite deleted the chore/scaffold-claim-msg branch October 23, 2023 20:24
okdas pushed a commit that referenced this pull request Nov 14, 2024
* chore: scaffold create claim message

`ignite scaffold message create-claim session_header:SessionHeader root_hash --signer SupplierAddress --module supplier`
(NB: generated a temporary SessionHeader duplicate in the supplier module so the above would work; subsequently, removed and updated protobuf import and usage.)

* refactor: use `bytes` type for `MsgCreateClaim#root_hash` field

* chore:  add TODO comment in msg handler

* chore: add TODOs

Co-authored-by: Daniel Olshansky <[email protected]>

---------

Co-authored-by: Daniel Olshansky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
miner Changes related to the Miner supplier Changes related to the Supplier actor
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[Miner] Foundation for the Miner submodule
3 participants