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.

Sign up for GitHub

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

Jump to bottom

[Supplier] Scaffold the Proof type and adjust some of the defaults #197

Merged
merged 20 commits into from
Nov 24, 2023
Merged

[Supplier] Scaffold the Proof type and adjust some of the defaults #197

merged 20 commits into from
Nov 24, 2023

Conversation

Olshansk
Copy link
Member

@Olshansk Olshansk commented Nov 17, 2023 •
edited by reviewpad bot
Loading

Summary

Scaffolding the Proof type + a few minor edits such as:

  • Removing proofs from the genesis state
  • Fixing the language around retrieving more than one proof
  • scaffold command:
$ ignite scaffold map proof --module supplier supplier_address session_id merkle_proof --no-message --yes

Human Summary

AI Summary

Summary generated by Reviewpad on 24 Nov 23 21:57 UTC

This pull request contains multiple file diffs related to the changes made in the codebase. Here is a summary of the changes:

  1. The file x/supplier/client/cli/query_proof_test.go has been added. It includes package declaration, commented out functions, and some code related to strconv that is commented out.

  2. The file x/supplier/client/cli/query.go has changes related to module configuration and scaffold operation. It includes import changes and the addition of new commands.

  3. The file x/supplier/client/cli/query.go introduces new endpoints, response schemas, and query parameters related to querying proofs.

  4. The file x/supplier/client/cli/query.go contains two new functions related to querying proofs, CmdListProof and CmdShowProof, along with a TODO comment.

  5. The file x/supplier/client/cli/query.go includes changes in import statements, removing unused imports, and organizing import statements.

  6. The file codec.go includes changes in import statements and a comment line for starport scaffolding.

  7. The file x/supplier/types/key_proof.go adds the ProofKey function and ProofKeyPrefix constant.

  8. The file genesis.go in the x/supplier/types package has changes related to the removal of a line and a comment for starport scaffolding.

  9. The file proof.proto is a new file that defines a message named Proof with four string fields.

  10. The file x/supplier/keeper/proof.go introduces functions related to managing proofs in the store.

  11. The file codec.go in the x/supplier/types package includes changes in import statements and the addition of message types to a function.

  12. The file codec.go adds an import statement for the "github.com/pokt-network/poktroll/x/application/types" package.

  13. The file codec.go includes changes related to imports and a function definition.

  14. The file proof_test.go in the x/supplier/keeper package contains test functions for the Proof type.

  15. The file proof_test.go includes test cases for querying proofs in the supplier module.

Please review these changes and let me know if you need further assistance with any specific diff.

Issue

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_develop_and_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

@Olshansk Olshansk added supplier Changes related to the Supplier actor protocol General core protocol related changes labels Nov 17, 2023
@Olshansk Olshansk added this to the Shannon TestNet milestone Nov 17, 2023
@Olshansk Olshansk self-assigned this Nov 17, 2023
@Olshansk Olshansk changed the title Add a comment on scaffolding [Supplier] Scaffold the Proof type and adjust some of the defaults Nov 17, 2023
@Olshansk Olshansk force-pushed the issues/141/scaffold_proof_type branch from 4952c56 to cf62672 Compare November 17, 2023 02:21
@Olshansk Olshansk force-pushed the issues/141/scaffold_proof_type branch from cf62672 to 9a9ca7a Compare November 17, 2023 02:31
@Olshansk Olshansk marked this pull request as ready for review November 17, 2023 02:31
h5law
h5law approved these changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@h5law h5law left a comment

Choose a reason for hiding this comment

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

Overall LGTM lets get it in :shipit:

go.mod
// types and functions will be scaffolded correctly.
// Ref: https://github.com/ignite/cli/issues/3737
//
// The following will need to be done manually after the scaffold:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a good time for a bash script IMO

Copy link
Contributor

@bryanchriswhite bryanchriswhite Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Somewhat tangential: I would like to propose that we entertain the idea of preferring to write scripts in go. I think the stdlib is more than capable on it's own to accomplish most scripting needs. While it may be verbose in some cases, I think minimizing the number of languages in the repo is worth it on the whole.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Pantani responded to it, so I want to see if we can solve it via the ignite path first before investing time into it writing proprietary scripts.

x/pocket/types/codec.go
@@ -1,10 +1,11 @@
package types

import (
// this line is used by starport scaffolding # 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this you or ignite moving this?

Copy link
Contributor

@bryanchriswhite bryanchriswhite Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Was this you or ignite moving this?

Or maybe the linter?

+1 and same question for similar moved ignite comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the linter.

x/supplier/client/cli/query_proof_test.go Outdated Show resolved Hide resolved
bryanchriswhite
bryanchriswhite reviewed Nov 22, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
bryanchriswhite
bryanchriswhite reviewed Nov 22, 2023
View reviewed changes
x/supplier/types/codec.go Outdated
Comment on lines 34 to 35

msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker but just curious if we know what's going on here. It looks redundant at first glance here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

This is a result of the pocket/github.com/pokt-network/potroll translation.

  1. The first one is intended.
  2. The second one was a result of not deleting it after the Claim scaffolding.
  3. The third one is a result of the Proof scaffolding

I'm deleting (2) & (3).

bryanchriswhite
bryanchriswhite requested changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

Nice one @Olshansk! 🙌

I left some comments, mostly goimports, typos, etc.: little things. A couple of questions as well.

image

Olshansk and others added 10 commits November 22, 2023 16:08
@Olshansk @bryanchriswhite
Update go.mod
3b491a5 
Co-authored-by: Bryan White <[email protected]>
@Olshansk @bryanchriswhite
Update x/supplier/client/cli/query_proof.go
b680d9c 
Co-authored-by: Bryan White <[email protected]>
@Olshansk @bryanchriswhite
Update x/supplier/client/cli/query_proof_test.go
33f7fd1 
Co-authored-by: Bryan White <[email protected]>
@red-0ne @Olshansk
[Supplier] feat: Add staking flag and config parser (#185)
cede36c 
* feat: Add staking flag and config parser

* chore: Update make file staking operation

* test: Update staking tests to use a json config

* test: add json staking config file

* feat: Use yaml format instead of json

* fix: Use yaml tags

* chore: Address change requests, Add tests

* fix: Add full path to supplier config

* chore: address change requests
@bryanchriswhite @Olshansk
[Miner] test: add #MinedRelays() coverage (#191)
df6ee28 
* test: improve curl usage in e2e suite

* chore: add `WithDifficulty()` miner option

* test: add `miner#MinedRelays` coverage

* chore: review feedback improvements

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

* refactor: `TestFixtureGeneration_MineMockRelays()`

add `--gen-mined-relays` flag to conditionally skip

* refactor: set fixture generation precedent

* chore: improve naming & comments

* chore: improve comments & cleanup

* chore: remove commented code

* refactor: `HashBytes` to shared testutil

* refactor & fix: difficulty

* [RelayMiner, Testing] test: add coverage to `relayerSessionsManager` (#194)

* test: `relayerSessionsManager` claim/proof life-cycle

* chore: review feedback improvements

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

---------

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

* chore: fix comment

* chore: review feedback improvements

* chore: review feedback improvements

* chore: review feedback improvements

* revert: `RelayRequest` pointer receivers

* chore: add TODO comment

* chore: add "DO NOT EDIT..." comment to fixture output

* chore: chore: review feedback improvements

---------

Co-authored-by: Daniel Olshansky <[email protected]>
@bryanchriswhite @Olshansk
chore: omitted improvments to #191 (#214)
6a21392
@Olshansk
Follow up on review comments
ef7afb5
@Olshansk
A couple more nites
7dfd86c
@Olshansk
Merge with main
d72738b
@Olshansk
Manually ran goimports -w -local github.com/pokt-network/poktroll on …
bdf0ee9 
…a couple of files
bryanchriswhite
bryanchriswhite approved these changes Nov 24, 2023
View reviewed changes
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

🚢

@Olshansk Olshansk merged commit df37f86 into main Nov 24, 2023
9 checks passed
@Olshansk Olshansk deleted the issues/141/scaffold_proof_type branch November 24, 2023 22:17
@bryanchriswhite bryanchriswhite linked an issue Dec 5, 2023 that may be closed by this pull request
[Protocol] MVP implementation off the on-chain SubmitProof message handling #141
Closed
8 tasks
okdas pushed a commit that referenced this pull request Nov 14, 2024
@Olshansk @bryanchriswhite @red-0ne
[Supplier] Scaffold the Proof type and adjust some of the defaults (#…
2cf12c0 
…197)

Scaffolding the `Proof` type + a few minor edits such as:
- Removing proofs from the genesis state
- Fixing the language around retrieving more than one proof
- scaffold command:
```bash
$ ignite scaffold map proof --module supplier supplier_address session_id merkle_proof --no-message --yes
```

---------

Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Redouane Lakrache <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h5law h5law h5law approved these changes

@bryanchriswhite bryanchriswhite bryanchriswhite approved these changes

Assignees

@Olshansk Olshansk

Labels
protocol General core protocol related changes supplier Changes related to the Supplier actor
Projects
Shannon
Status: ✅ Done
Milestone
Shannon Private TestNet
Development

Successfully merging this pull request may close these issues.

[Protocol] MVP implementation off the on-chain SubmitProof message handling
4 participants
@Olshansk @bryanchriswhite @h5law @red-0ne