feat: specify share version in MsgWirePayForBlob
#1028
Conversation
rootulp
force-pushed
the
rp/specify-share-version
branch
from
aeacb80
to
fe097d9
Compare
Codecov Report
@@ Coverage Diff @@
## main #1028 +/- ##
==========================================
+ Coverage 50.88% 50.97% +0.09%
==========================================
Files 71 71
Lines 4398 4400 +2
==========================================
+ Hits 2238 2243 +5
+ Misses 1936 1932 -4
- Partials 224 225 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
thanks for adding this
[comment][no change]
tbh it feels weird that we're adding the option to add specify a share version, but we only support a single share version. I know we still have to add it, but it feels like a potential source of confusion in the future given that only a few people know what it is.
[comment][no change]
For example, I'm not yet convinced user facing functions, such as celestia-node or our cli, should expose it in their API yet. Perhaps only as an optional flag, but maybe not as a required arg
FWIW I don't think it needs to be exposed when there only exists one supported share version but I do think it needs to be exposed at the time of introducing another share version format.
Agreed completely. #1041 intends on making it optional with a sane default (e.g. |
Agreed. This PR adds |
Absolutely no worries, I think I just fixed all issues so this is ready for re-review |
rootulp
changed the title
MsgWirePayForBlob
|
Hold off on merging, while working on an unrelated change I discovered https://github.com/rootulp/celestia-app/blob/26d800681fa9b3ae90ae9b9ad6600dc5e0d5247d/pkg/shares/utils.go#L44-L64 which doesn't account for the newly added share version field Update: resolved by ce73010 |
evan-forbes
added
C: Celestia app
consensus breaking
Follow up to #1028 where I didn't propagate the new struct field `ShareVersion` to all instances of `coretypes.Blob`. Unfortunately Go isn't exhaustive on specifying struct fields so I wasn't made aware of this miss until working on an unrelated change. Open to ideas on how to have higher confidence all instances have been addressed. [The non_exhaustive attribute](https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute) makes me think Rust structs are exhaustive by default.
Follow up to celestiaorg/celestia-app#1028 where I didn't propagate the new struct field `ShareVersion` to all instances of `coretypes.Blob`. Unfortunately Go isn't exhaustive on specifying struct fields so I wasn't made aware of this miss until working on an unrelated change. Open to ideas on how to have higher confidence all instances have been addressed. [The non_exhaustive attribute](https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute) makes me think Rust structs are exhaustive by default.
Follow up to celestiaorg/celestia-app#1028 where I didn't propagate the new struct field `ShareVersion` to all instances of `coretypes.Blob`. Unfortunately Go isn't exhaustive on specifying struct fields so I wasn't made aware of this miss until working on an unrelated change. Open to ideas on how to have higher confidence all instances have been addressed. [The non_exhaustive attribute](https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute) makes me think Rust structs are exhaustive by default.
Closes #936
Opens #1041