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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic AppState for genesis() #1107

Merged
merged 4 commits into from
Mar 21, 2022
Merged

Conversation

hu55a1n1
Copy link
Member

@hu55a1n1 hu55a1n1 commented Mar 17, 2022

Resolves: #1106

This PR basically modifies the genesis() RPC function like so ->

-    async fn genesis(&self) -> Result<Genesis, Error> 
+    async fn genesis<AppState>(&self) -> Result<Genesis<AppState>, Error>

(Many thanks to @romac for writing the code 馃檹)

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Added entry in .changelog/

@@ -30,6 +30,5 @@ pub struct Genesis<AppState = serde_json::Value> {
pub app_hash: Vec<u8>,

/// App state
#[serde(default)]
Copy link
Member

Choose a reason for hiding this comment

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

I removed this because it would otherwise introduce a Default bound on AppState.

Now, did we expect the app_state property to be missing from the response sometimes? If so, then we should revert that change.

Copy link
Member Author

@hu55a1n1 hu55a1n1 Mar 17, 2022

Choose a reason for hiding this comment

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

I think users might still be able to use an Genesis<Option<AppState>> as a workaround in such cases. 馃

Copy link
Member Author

Choose a reason for hiding this comment

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

@hu55a1n1 hu55a1n1 marked this pull request as ready for review March 17, 2022 19:18
@thanethomson thanethomson added the backport:v0.24 This PR is to be back/front-ported to v0.24.x label Mar 21, 2022
Copy link
Member

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

LGTM 馃憤

@hu55a1n1 are you planning on applying this same change to master? Let me know if you need assistance there.

@thanethomson thanethomson merged commit 592bfda into v0.23.x Mar 21, 2022
@thanethomson thanethomson deleted the hu55a1n1/1106-genesis-app-state branch March 21, 2022 17:39
james-chf pushed a commit to heliaxdev/tendermint-rs that referenced this pull request Sep 30, 2022
* Allow users to supply generic param AppState in genesis()

* Add changelog entry

* Fix tests

* Fix clippy errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:v0.24 This PR is to be back/front-ported to v0.24.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants