-
Notifications
You must be signed in to change notification settings - Fork 970
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
refactoring!(state): extend SubmitPayForBlob method #1963
refactoring!(state): extend SubmitPayForBlob method #1963
Conversation
Codecov Report
@@ Coverage Diff @@
## feature_branch_blob_module #1963 +/- ##
=============================================================
Coverage ? 54.55%
=============================================================
Files ? 216
Lines ? 14021
Branches ? 0
=============================================================
Hits ? 7649
Misses ? 5553
Partials ? 819 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about adding app types to the API. This means that someone wanting to use our client will now need to also import app. This endpoint was already a bit difficult to use, because users need to construct a namespace.ID
, and the fee parameter is a string (math.Int
), while gasLim is a uint64. This is just bad UX :/
I am wondering what the best alternative would be. I would handle the complexity inside of node and maybe create our own simple blob type to avoid imports for the user. Do users really need to know about the ShareVersion?
Do you have thoughts on changing the signature to
func SubmitPayForBlobs(ctx context.Context, fee, gasLim uint64, blobs ...blob.Blob) (*state.TxResponse, error)
?
|
My idea was to merge this one and then update it after introducing the blob struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually prefer we hold off on breaking API right now (at least before next release).
70c34f0
to
6b5aabf
Compare
Created a feature branch for the Blob Module. So, I can merge everything there and then we can test it |
I agree here with you. We've discussed with Evan extracting the Blob type to a separate repo, so, we can re-use it. But it's not a high priority rn. I'd suggest leaving app types for now, keeping in mind that they will be changed in the future. (After extracting we can rework basic types) |
Overview
Resolves #1840
Checklist