feat: add signers to the TxResponse #683
Conversation
ninabarbakadze
changed the title
ninabarbakadze
requested review from
rootulp and
tzdybal
and removed request for
a team
|
@ninabarbakadze your pull request is missing a changelog! |
There was a problem hiding this comment.
I looked into the proto generation and built images/simd-dlv/Dockerfile locally with the buf version specified in upstream and our required go toolchain version.
Also bumped the image version in the makefile to latest available proto-gen image but it's still behind for us so I reverted it therefore this only works for me if you build the image locally and tag it as what makefile expects.
| // | ||
| // Since: cosmos-sdk 0.42.11, 0.44.5, 0.45 | ||
| repeated tendermint.abci.Event events = 13 [(gogoproto.nullable) = false]; | ||
| // The signers of the transaction. |
There was a problem hiding this comment.
[question] How is each signer represented in this [][]byte? In celestia-app we often use the signer's address as a string but given this isn't a repeated string, I assume it's a slice of the raw bytes of each signer (?)
Maybe this comment can clarify the format of signers.
There was a problem hiding this comment.
Yes it's a slice of raw bytes of each signer buy now that I'm thinking I think it'd be better to parse it to strings before we return to the user so might just change it to strings and I'll update the comment too.
There was a problem hiding this comment.
I updated it to strings.
Some of the things failing in CI I could not resolve :/ I spoke to binary team and the said that the CI on the sdk fork was never fully addressed from v50 upgrade. I'll ask Julien again about the CI failures. |
x/staking/types/staking.pb.go
Outdated
| @@ -1404,803 +1404,789 @@ func (this *Pool) Description() (desc *github_com_cosmos_gogoproto_protoc_gen_go | |||
| func StakingDescription() (desc *github_com_cosmos_gogoproto_protoc_gen_gogo_descriptor.FileDescriptorSet) { | |||
| d := &github_com_cosmos_gogoproto_protoc_gen_gogo_descriptor.FileDescriptorSet{} | |||
| var gzipped = []byte{ | |||
| // 12723 bytes of a gzipped FileDescriptorSet | |||
There was a problem hiding this comment.
do we know what the diff was here and if its consensus breaking?
There was a problem hiding this comment.
I seriously doubt that it is, but always good to know why this portion changes. most of the time its just a godoc etc
There was a problem hiding this comment.
Yeah I'm not gonna lie I don't really understand why staking protos were updated. I can ask around.
There was a problem hiding this comment.
Actually prolly because staking proto file was updated before and protos were not regenerated
evan-forbes
left a comment
evan-forbes
left a comment
There was a problem hiding this comment.
can't approve my own PR but I approve
|
we might be able to easily fix some of the CI if we bump stuff go versions etc we don't need to block imo but should still fix or disable if we don't find it useful |
I will approve on your behalf then. I opened the issue to track fixing CI and we can prio it during sync #688 |
rootulp
left a comment
rootulp
left a comment
There was a problem hiding this comment.
[not blocking]
- We can prob add to the README that TxResponse was extended to include
signers - Agreed it seems weird that
staking.pb.gochanged and also agree that it could the relevant.protofile have been changed in a previous PR but someone forgot to include the updated generated.pb.go. If we want to avoid this in the future we could add CI that fails ifmake proto-genmakes a git diff.
|
ninabarbakadze
merged commit 6e5c782
into
release/v0.51.x-celestia
adding signers to the TxResponse so it can be populated in TxClient