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

Adds remaining messages and queries to QGB skeleton scaffolding #212

Merged
merged 10 commits into from
Feb 16, 2022
Merged

Adds remaining messages and queries to QGB skeleton scaffolding #212

merged 10 commits into from
Feb 16, 2022

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Feb 15, 2022

Adds the remaining skeleton for #197

@rach-id rach-id changed the title Adds remaining messages and queries to QGB scaffolding Adds remaining messages and queries to QGB skeleton scaffolding Feb 15, 2022
@rach-id
Copy link
Member Author

rach-id commented Feb 15, 2022 •
edited

@evan-forbes Please take a look when you have time.
If the structure is right and it's the right skeleton. Then, I can open other PRs filling the logic.
Thanks.

evan-forbes
evan-forbes reviewed Feb 15, 2022
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment •
edited

Choose a reason for hiding this comment

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

I'm still comparing this to the gravity module, but I think it would be better to target a different branch other than master, since there is so much unfinished functionality. This is likely what we should have done with the first skeleton PR. If we target some qgb feature branch, then we can continue with these smaller PRs and tighter feedback loops, which I think are good.

overall this is very much headed in the right direction!

proto/qgb/msgs.proto Outdated Show resolved Hide resolved
x/qgb/keeper/keeper_delegate_key.go Outdated Show resolved Hide resolved
x/qgb/keeper/query_delegate_key.go Outdated Show resolved Hide resolved
@rach-id
Copy link
Member Author

rach-id commented Feb 15, 2022

@evan-forbes Sure we can do that. And also we can revert the previously merged branch and put 8t in a separate one.
Agreed. It makes more sense that way.

@evan-forbes
Copy link
Member

cool, just pushed the qgb-integration branch

@rach-id
Copy link
Member Author

rach-id commented Feb 15, 2022

Awesome. Thanks a lot

@rach-id rach-id changed the base branch from master to qgb-integration February 15, 2022 17:57
evan-forbes
evan-forbes reviewed Feb 15, 2022
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

alrighty, I think these changes make sense and we can merge into the qgb-integration branch after we address the remaining comments and tests! 👍 LGTM

proto/qgb/query.proto Outdated Show resolved Hide resolved
x/qgb/keeper/query_valset_confirm.go Outdated Show resolved Hide resolved
@rach-id
Copy link
Member Author

rach-id commented Feb 16, 2022

@evan-forbes can we merge this now ?

evan-forbes
evan-forbes approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

yep! nice work 🚢

@evan-forbes evan-forbes merged commit 7c0e453 into celestiaorg:qgb-integration Feb 16, 2022
@rach-id rach-id deleted the qgb_module_scaffolding branch February 16, 2022 20:24
@evan-forbes evan-forbes mentioned this pull request Feb 20, 2022
Closed
rach-id added a commit to rach-id/celestia-app that referenced this pull request May 9, 2022
@rach-id
Adds remaining messages and queries to QGB skeleton scaffolding (cele…
4149fe7 
…stiaorg#212)

* removes dummy field from data commitment

* adds TODOs

* adds comments

* linter fixes

* linter fixes

* linter fixes

* linter fixes

* linter fixes

* CI fixes

* query rename
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rach-id @evan-forbes