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

refactor: Move some methods inside TX Factory #9421

Merged
merged 10 commits into from
Jun 28, 2021
Merged

Conversation

jgimeno
Copy link
Contributor

@jgimeno jgimeno commented May 28, 2021

Description

Putting some things inside the factory (which was very anemic struct) has helped me to understand the flow. Feel free to merge if you see some benefit.

closes: #XXXX


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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
  • Review Codecov Report in the comment section below once CI passes

@github-actions github-actions bot added the C:x/genutil genutil module issues label May 28, 2021
@codecov
Copy link

codecov bot commented May 28, 2021

Codecov Report

Merging #9421 (179d9e1) into master (0027111) will increase coverage by 25.22%.
The diff coverage is 62.90%.

❗ Current head 179d9e1 differs from pull request most recent head 482edf6. Consider uploading reports for the commit 482edf6 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #9421       +/-   ##
===========================================
+ Coverage   35.48%   60.70%   +25.22%     
===========================================
  Files         332      588      +256     
  Lines       32620    37277     +4657     
===========================================
+ Hits        11575    22629    +11054     
+ Misses      19819    12698     -7121     
- Partials     1226     1950      +724     
Impacted Files Coverage Δ
client/keys/show.go 83.67% <ø> (-0.24%) ⬇️
client/keys/types.go 100.00% <ø> (+100.00%) ⬆️
client/keys/utils.go 42.85% <ø> (+2.50%) ⬆️
client/query.go 16.98% <ø> (ø)
client/rpc/block.go 10.00% <ø> (ø)
client/rpc/routes.go 100.00% <ø> (ø)
client/rpc/status.go 47.72% <ø> (ø)
client/rpc/validators.go 0.00% <ø> (ø)
client/test_helpers.go 0.00% <ø> (ø)
client/tx/factory.go 27.00% <ø> (ø)
... and 699 more

@jgimeno jgimeno changed the title Move some methods inside TX Factory refactor: Move some methods inside TX Factory May 28, 2021
@jgimeno jgimeno marked this pull request as ready for review May 28, 2021 15:33
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

I am a fan of this change!! utACK

Could you add a changelog entry please?

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Seems like an API improvement to me too. Could you add a changelog entry?

Since this is an API-breaking change, I would rather wait for 0.43-rc to be out before merging.

@tac0turtle
Copy link
Member

Since this is an API-breaking change, I would rather wait for 0.43-rc to be out before merging.

Whats the difference with getting this in before vs after?

@amaury1093
Copy link
Contributor

amaury1093 commented Jun 2, 2021

We decided in #9116 to freeze master of unnecessary API breaking changes. Once the beta phase is done, we'll create a release/v0.43.x branch, so normal dev work (like this PR) can go to master again.

@aaronc
Copy link
Member

aaronc commented Jun 2, 2021

Since this is an API-breaking change, I would rather wait for 0.43-rc to be out before merging.

Whats the difference with getting this in before vs after?

We're trying our best to stick to the principle of a beta actually being a beta. We didn't with Stargate and that extended the release timeline IMHO...

@tac0turtle
Copy link
Member

We're trying our best to stick to the principle of a beta actually being a beta. We didn't with Stargate and that extended the release timeline IMHO...

Makes sense, could we get the contributing.md file updated to reflect this process.

@blushi blushi added the S: DO NOT MERGE Status: DO NOT MERGE label Jun 17, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

This can finally be merged! @jgimeno could you add a CHangelog entry too?

@amaury1093 amaury1093 removed the S: DO NOT MERGE Status: DO NOT MERGE label Jun 28, 2021
@jgimeno
Copy link
Contributor Author

jgimeno commented Jun 28, 2021

Absolutely!

@jgimeno jgimeno requested a review from amaury1093 June 28, 2021 11:11
CHANGELOG.md Outdated Show resolved Hide resolved
@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 28, 2021
@mergify mergify bot merged commit e17be87 into master Jun 28, 2021
@mergify mergify bot deleted the jonathan/tx-cleanup branch June 28, 2021 11:42
@robert-zaremba
Copy link
Collaborator

@Mergifyio backport release/v0.44.x

mergify bot pushed a commit that referenced this pull request Aug 31, 2021
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
v    Before smashing the submit button please review the checkboxes.
v    If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

## Description

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review.
-->

Putting some things inside the factory (which was very anemic struct) has helped me to understand the flow. Feel free to merge if you see some benefit.

closes: #XXXX

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit e17be87)

# Conflicts:
#	CHANGELOG.md
@mergify
Copy link
Contributor

mergify bot commented Aug 31, 2021

Command backport release/v0.44.x: success

Backports have been created

robert-zaremba added a commit that referenced this pull request Aug 31, 2021
* refactor: Move some methods inside TX Factory (#9421)

<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
v    Before smashing the submit button please review the checkboxes.
v    If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

## Description

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review.
-->

Putting some things inside the factory (which was very anemic struct) has helped me to understand the flow. Feel free to merge if you see some benefit.

closes: #XXXX

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit e17be87)

# Conflicts:
#	CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
@robert-zaremba robert-zaremba mentioned this pull request Sep 1, 2021
19 tasks
robert-zaremba added a commit that referenced this pull request Sep 1, 2021
robert-zaremba added a commit that referenced this pull request Sep 1, 2021
* Revert "refactor: Move some methods inside TX Factory (backport #9421) (#10039)"

This reverts commit 0155244.

* remove conflict marks from changelog

* update changelog
evan-forbes pushed a commit to evan-forbes/cosmos-sdk that referenced this pull request Oct 12, 2021
…osmos#10039)

* refactor: Move some methods inside TX Factory (cosmos#9421)

<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
v    Before smashing the submit button please review the checkboxes.
v    If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

## Description

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review.
-->

Putting some things inside the factory (which was very anemic struct) has helped me to understand the flow. Feel free to merge if you see some benefit.

closes: #XXXX

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit e17be87)

# Conflicts:
#	CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
evan-forbes pushed a commit to evan-forbes/cosmos-sdk that referenced this pull request Oct 12, 2021
* Revert "refactor: Move some methods inside TX Factory (backport cosmos#9421) (cosmos#10039)"

This reverts commit 0155244.

* remove conflict marks from changelog

* update changelog
evan-forbes pushed a commit to evan-forbes/cosmos-sdk that referenced this pull request Nov 1, 2021
…osmos#10039)

* refactor: Move some methods inside TX Factory (cosmos#9421)

<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
v    Before smashing the submit button please review the checkboxes.
v    If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

## Description

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review.
-->

Putting some things inside the factory (which was very anemic struct) has helped me to understand the flow. Feel free to merge if you see some benefit.

closes: #XXXX

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit e17be87)

# Conflicts:
#	CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
evan-forbes pushed a commit to evan-forbes/cosmos-sdk that referenced this pull request Nov 1, 2021
* Revert "refactor: Move some methods inside TX Factory (backport cosmos#9421) (cosmos#10039)"

This reverts commit 0155244.

* remove conflict marks from changelog

* update changelog
Eengineer1 pushed a commit to cheqd/cosmos-sdk that referenced this pull request Aug 26, 2022
…osmos#10039)

* refactor: Move some methods inside TX Factory (cosmos#9421)

<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
v    Before smashing the submit button please review the checkboxes.
v    If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review.
-->

Putting some things inside the factory (which was very anemic struct) has helped me to understand the flow. Feel free to merge if you see some benefit.

closes: #XXXX

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit e17be87)

* Update CHANGELOG.md

Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Eengineer1 pushed a commit to cheqd/cosmos-sdk that referenced this pull request Aug 26, 2022
* Revert "refactor: Move some methods inside TX Factory (backport cosmos#9421) (cosmos#10039)"

This reverts commit 0155244.

* remove conflict marks from changelog

* update changelog
JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Sep 28, 2024
…osmos#10039)

* refactor: Move some methods inside TX Factory (cosmos#9421)

<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
v    Before smashing the submit button please review the checkboxes.
v    If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

## Description

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review.
-->

Putting some things inside the factory (which was very anemic struct) has helped me to understand the flow. Feel free to merge if you see some benefit.

closes: #XXXX

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit e17be87)

# Conflicts:
#	CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Sep 28, 2024
* Revert "refactor: Move some methods inside TX Factory (backport cosmos#9421) (cosmos#10039)"

This reverts commit 0155244.

* remove conflict marks from changelog

* update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:x/auth C:x/genutil genutil module issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants