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

regeneration of app #127

Merged
merged 18 commits into from
Oct 7, 2021
Merged

regeneration of app #127

merged 18 commits into from
Oct 7, 2021

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Sep 29, 2021

Description

This PR regenerates the app using the latest version of starport, our fork of the sdk based on v0.44.0, and our version of celestia-core that is based on v0.34.12 of tendermint.

I tried to limit the changes in this PR to strictly regenerating/upgrading and not refactoring, which we still need to do.

closes #114
blocked by #33/#34

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2021

Codecov Report

Merging #127 (0ebad95) into master (18e69b5) will decrease coverage by 0.09%.
The diff coverage is 62.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
- Coverage   19.58%   19.49%   -0.10%     
==========================================
  Files          15       14       -1     
  Lines        2783     2750      -33     
==========================================
- Hits          545      536       -9     
+ Misses       2211     2185      -26     
- Partials       27       29       +2     
Impacted Files Coverage Δ
app/encoding.go 0.00% <0.00%> (-100.00%) ⬇️
app/export.go 0.00% <ø> (ø)
x/payment/types/codec.go 0.00% <0.00%> (ø)
x/payment/types/genesis.pb.go 1.69% <0.00%> (+0.02%) ⬆️
x/payment/types/keys.go 0.00% <ø> (ø)
x/payment/types/tx.pb.go 1.05% <0.00%> (+0.01%) ⬆️
x/payment/client/builder.go 41.81% <33.33%> (ø)
app/app.go 82.94% <83.63%> (+0.92%) ⬆️
app/abci.go 66.66% <100.00%> (+1.19%) ⬆️
app/genesis.go 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18e69b5...0ebad95. Read the comment docs.

Comment on lines +90 to +106
// pfmURL is the URL expected for pfm. NOTE: this will be deleted when we upgrade from
// sdk v0.44.0
var pfmURL = sdk.MsgTypeURL(&types.MsgWirePayForMessage{})

func hasWirePayForMessage(tx sdk.Tx) bool {
for _, msg := range tx.GetMsgs() {
if msg.Type() == types.TypeMsgPayforMessage {
msgName := sdk.MsgTypeURL(msg)
if msgName == pfmURL {
return true
}
// note: this is what we will use in the future as proto.MessageName is
// deprecated
// svcMsg, ok := msg.(sdk.ServiceMsg) if !ok {
// continue
// } if svcMsg.SerivceMethod == types.TypeMsgPayforMessage {
// return true
// }
Copy link
Member Author

Choose a reason for hiding this comment

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

this will have to be replaced as soon as we use a newer version of the sdk (see comment).

go.mod Outdated Show resolved Hide resolved
option go_package = "github.com/celestiaorg/celestia-core/abci/types";
option go_package = "github.com/tendermint/tendermint/abci/types";
Copy link
Member Author

Choose a reason for hiding this comment

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

this closes #145! 🙌 This is actually really nice, as we will be able to use the cli in scripts w/o getting messed up output. i.e

($celestia-appd tendermint show-node-id)

expected: []byte{0x5d, 0x43, 0xd7, 0x40, 0xe5, 0xe6, 0x5e, 0x2a, 0xb9, 0x10, 0x5c, 0xf9, 0x26, 0xf9, 0xf0, 0x1c, 0x3a, 0x11, 0x49, 0x1c, 0x71, 0x21, 0xdf, 0x46, 0xdd, 0x21, 0x94, 0x3f, 0xba, 0xb1, 0xcf, 0xd4},
expected: []byte{0x1c, 0x57, 0x89, 0x2f, 0xbe, 0xbf, 0xa2, 0xa4, 0x4c, 0x41, 0x9e, 0x2d, 0x88, 0xd5, 0x87, 0xc0, 0xbd, 0x37, 0xc0, 0x85, 0xbd, 0x10, 0x3c, 0x36, 0xd9, 0xa2, 0x4d, 0x4e, 0x31, 0xa2, 0xf8, 0x4e},
Copy link
Member Author

Choose a reason for hiding this comment

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

we updated to a new version of nmt, which includes the namespace in the hash, which changes this output

expected: []byte(`{"fee":{"base_rate_max":"10000","tip_rate_max":"1000"},"message_namespace_id":"AQIDBAECAwQ=","message_share_commitment":"byozRVIrw5NF/rU1PPyq6BAo3g2ny3uLTiOFedtgSwo=","message_size":"4","nonce":"1"}`),
expected: []byte(`{"fee":{"base_rate_max":"10000","tip_rate_max":"1000"},"message_namespace_id":"AQIDBAECAwQ=","message_share_commitment":"Elh5P8yB1FeiPP0uWCkp67mqSsaVat6iwjH2vSMQJys=","message_size":"4","nonce":"1"}`),
Copy link
Member Author

Choose a reason for hiding this comment

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

same as above, new version of nmt causes the commitments to be different

@evan-forbes evan-forbes marked this pull request as ready for review September 30, 2021 00:09
@evan-forbes evan-forbes changed the title regeneration/refactor of app regeneration of app Sep 30, 2021
Copy link
Member

@Bidon15 Bidon15 left a comment

Choose a reason for hiding this comment

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

Speaking of

  • x/payment/client/cli/query.go
  • x/payment/keeper/grpc_query.go
  • x/payment/keeper/errors.go
  • x/payment/keeper/expected_keepers.go
  • x/payment/types/types.go

Are they meant to be empty files?

app/app.go Outdated Show resolved Hide resolved
x/payment/genesis_test.go Outdated Show resolved Hide resolved
x/payment/keeper/msg_server_test.go Outdated Show resolved Hide resolved
x/payment/module.go Show resolved Hide resolved
x/payment/module.go Show resolved Hide resolved
Co-authored-by: Nguyen Nhu Viet <braveryandglory@gmail.com>
@evan-forbes
Copy link
Member Author

Are they meant to be empty files?

they're meant for scaffolding using starport, and if we end up using them, then we can add them back in. deleted in 0d0b88a

thanks!

Co-authored-by: Nguyen Nhu Viet <braveryandglory@gmail.com>
Bidon15
Bidon15 previously approved these changes Oct 4, 2021
Copy link
Member

@Bidon15 Bidon15 left a comment

Choose a reason for hiding this comment

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

Thank you for insightful answers. lgtm 🚢

Bidon15
Bidon15 previously approved these changes Oct 4, 2021
tzdybal
tzdybal previously approved these changes Oct 5, 2021
Copy link
Member

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

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

LGTM

liamsi
liamsi previously approved these changes Oct 6, 2021
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

LGTM

x/payment/types/codec.go Show resolved Hide resolved
x/payment/types/errors.go Outdated Show resolved Hide resolved
x/payment/types/payformessage.go Outdated Show resolved Hide resolved
x/payment/types/codec.go Show resolved Hide resolved
@liamsi
Copy link
Member

liamsi commented Oct 6, 2021

Do you have any idea why some of the files changed pems to executable?
image

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Going to be anal about this, but can the permissions be fixed back to 644 instead of 755? Text files shouldn't be executable!

@evan-forbes evan-forbes dismissed stale reviews from liamsi, tzdybal, and Bidon15 via 0ebad95 October 6, 2021 23:00
@evan-forbes
Copy link
Member Author

evan-forbes commented Oct 6, 2021

Do you have any idea why some of the files changed pems to executable?

Going to be anal about this, but can the permissions be fixed back to 644 instead of 755? Text files shouldn't be executable!

I'm super sorry, I vaguely remember changing permissions on some files after generating them in starport because it wouldn't let me do anything with them, and I clearly did not change them back/appropriately. permissions we're changed back in bcabdfa

thank you for catching that!!

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Thanks!

@evan-forbes evan-forbes merged commit d061ef0 into master Oct 7, 2021
Weekly Sprint (core/app/libs) automation moved this from In Review to Done Oct 7, 2021
@liamsi liamsi deleted the evan/regenerate-app branch October 7, 2021 13:18
rootulp added a commit to rootulp/celestia-app that referenced this pull request Jul 14, 2022
cosmos/keyring hasn't pulled upstream changes from 99designs/keyring yet
and the latest release of 99designs/keyring (1.2.1) appears to contain
a fix for MacOS warnings: 99designs/keyring#107

Links
- https://github.com/cosmos/keyring/tags
- https://github.com/99designs/keyring/releases/tag/v1.2.1

`make test` passes after this change and it looks like other Cosmos
chains don't need this replace:
- https://github.com/osmosis-labs/osmosis/blob/main/go.mod#L274-L285=
- https://github.com/CosmosContracts/juno/blob/main/go.mod#L178-L182=

Does anyone know if it's still necessary? I couldn't find the justification in
celestiaorg#127 which added it.
@rootulp rootulp mentioned this pull request Jul 14, 2022
rootulp added a commit that referenced this pull request Aug 11, 2022
cosmos/keyring hasn't pulled upstream changes from 99designs/keyring yet
and the latest release of 99designs/keyring (1.2.1) appears to contain
a fix for MacOS warnings: 99designs/keyring#107

Links
- https://github.com/cosmos/keyring/tags
- https://github.com/99designs/keyring/releases/tag/v1.2.1

`make test` passes after this change and it looks like other Cosmos
chains don't need this replace:
- https://github.com/osmosis-labs/osmosis/blob/main/go.mod#L274-L285=
- https://github.com/CosmosContracts/juno/blob/main/go.mod#L178-L182=

Does anyone know if it's still necessary? I couldn't find the justification in
#127 which added it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Use the latest version of tendermint and the sdk
6 participants