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

fix!: update the fee collector name to match Agoric usage #406

Closed
wants to merge 3 commits into from

Conversation

JimLarson
Copy link

@JimLarson JimLarson commented Mar 7, 2024

Description

Refs: Agoric/agoric-sdk#9036

Update the fee collector module account name to match Agoric usage.

This eliminates the need for Agoric/agoric-sdk to maintain its own clone of DeductFeeDecorator.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@JimLarson JimLarson added bug Something isn't working agoric-cosmos Agoric tags for cosmos labels Mar 7, 2024
@JimLarson JimLarson self-assigned this Mar 7, 2024
@JimLarson
Copy link
Author

Lint failure is expected.

I noticed that we're not following the CHANGELOG convention in CHANGELOG-Agoric - we should be adding a link to the PR, not to the issue. This means that you can't have the changelog entry when you create the PR since you need the number.

I always forget to add the changelog entry, so now that seems prescient.

Copy link

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

LGTM with a few comment suggestions. Is this PR targeting the right branch, or should we create a new one for this and #405?

*   a0154d6890 (Agoric) Merge pull request #404 from agoric-labs/jimlarson-lint
|\  
| * e96708b346 chore: silence some lint warnings
|/  
*   9c20eacace Merge pull request #385 from agoric-labs/8989-rm-lien
|\  
| * b3b9528ad5 docs: changelog
| * 28ed7db56c chore: restore vesting account LockedCoins signature
| * 41d452862b chore: remove gymnastics around vesting account actions
| * dd6bf90d83 chore: revert interface-ification of AccountKeeper
|/  
*   0d5eab2ce2 Merge pull request #383 from agoric-labs/8890-disable-sims
|\  
| * e592d3a4b0 tests: disable sims which always run too long
|/  
*   f42d86980d (tag: v0.46.16-alpha.agoric.2) Merge pull request #379 from agoric-labs/mhofman/backport-19371
|\  
| * 84d5de04d3 fix: avoid cli redundant log in stdout (#19371)
* | b0baff1a47 Merge pull request #378 from agoric-labs/8871-submit-proposal
|\| 
| * aa291d837c docs: update changelog
| * 5cec00b6ea feat!: have tx gov submit-proposal accept either new or legacy syntax
|/  
*   c243c2e43a (tag: v0.46.16-alpha.agoric.1) Merge pull request #376 from agoric-labs/8224-tagnotes

@@ -12,7 +12,8 @@ const (
StoreKey = "acc"

// FeeCollectorName the root string for the fee collector account address
FeeCollectorName = "fee_collector"
// Agoric: maintain compatibility with Agoric/agoric-sdk/golang/cosmos/x/vbank/types.ReservePoolName

Choose a reason for hiding this comment

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

Suggested change
// Agoric: maintain compatibility with Agoric/agoric-sdk/golang/cosmos/x/vbank/types.ReservePoolName
// Agoric: collect fees into the reserve pool
// cf. Agoric/agoric-sdk golang/cosmos/x/vbank/types/key.go `ReservePoolName`

@JimLarson
Copy link
Author

We'll follow the practice of landing changes on trunk first, then cherry-pick to the (to-be-created) release branch.

@JimLarson
Copy link
Author

#405 should be cherry-picked there too.

Following past precedent, which it's unnecessary to improve upon, we'll branch Agoric-v0.46.16.2.x off of v0.46.16-alpha.agoric.2 and lay down the tag v0.46.16-alpha.agoric.2.1.

@JimLarson
Copy link
Author

Won't work due to other functionality that automatically distributes the fee collector account - closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agoric-cosmos Agoric tags for cosmos bug Something isn't working C:x/auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants