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

Improve client-side tx UX #6034

Closed
wants to merge 9 commits into from
Closed

Improve client-side tx UX #6034

wants to merge 9 commits into from

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Apr 20, 2020

ref: #6030

Implements the changes in #6014

/cc: @alexanderbez


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@aaronc
Copy link
Member Author

aaronc commented Apr 20, 2020

@alexanderbez I'm realizing that codec/std isn't the right place for StdFee, etc. codec/std depends on all the modules whereas StdFee, etc. should depend on none. How about either types/ or a new package like types/tx, std/tx, or just tx?

On a related note, seems like we should be moving codec/std -> std since that new package now exists, right?

@alexanderbez
Copy link
Contributor

alexanderbez commented Apr 20, 2020

Those types (stdfee, etc...) should live under the root /std pkg, that's why we created that. Codec stuff can remain under codec/std.

@aaronc
Copy link
Member Author

aaronc commented Apr 20, 2020

Those types (stdfee, etc...) should live under the root /std pkg, that's why we created that. Codec stuff can remain under codec/std.

So when I initially proposed /std I was intending for the codec/std types to move there. As the godoc says:

Package std defines all the common and standard inter-module Cosmos SDK types

So that means stuff in codec/std because they are inter-module. I think it will be pretty confusing if we have both codec/std and std. So I suggest we choose one of the other and I'd vote for simply std with the proto package cosmos_sdk.std.v1.

But StdFee, etc. do not depend on any other modules just types/ so they are misplaced there IMHO. In keeping with a package defined by what it is used for, they should be in a some sort of standard tx package. Probably that should have the proto package cosmos_sdk.tx.v1 regardless of what its go package is (that's easier to move around).

As we move towards releasing this stuff we should think carefully through the proto packages because anything we release with a v1 we're sort of making the promise not to make breaking changes to. When in doubt we should opt for v1alpha or v1beta. Maybe this deserves its own issue to track.

@alexanderbez
Copy link
Contributor

So that means stuff in codec/std because they are inter-module. I think it will be pretty confusing if we have both codec/std and std. So I suggest we choose one of the other and I'd vote for simply std with the proto package cosmos_sdk.std.v1.

👍

But StdFee, etc. do not depend on any other modules just types/ so they are misplaced there IMHO. In keeping with a package defined by what it is used for, they should be in a some sort of standard tx package. Probably that should have the proto package cosmos_sdk.tx.v1 regardless of what its go package is (that's easier to move around).

Why not just have them under /types?

@aaronc aaronc mentioned this pull request Apr 27, 2020
11 tasks
@aaronc aaronc closed this May 15, 2020
@tac0turtle tac0turtle deleted the aaronc/improve-signing-ux branch March 4, 2021 15:55
@tac0turtle tac0turtle restored the aaronc/improve-signing-ux branch March 4, 2021 15:55
@alessio alessio deleted the aaronc/improve-signing-ux branch March 14, 2021 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants