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

Update to cosmos-v0.37.0 #231

Merged
merged 1 commit into from
Sep 17, 2019
Merged

Conversation

yun-yeo
Copy link
Contributor

@yun-yeo yun-yeo commented Sep 3, 2019

Summary of changes

Update cosmos-sdk v0.37.0

  • All REST responses now wrap the original resource/result. The response
    will contain two fields: height and result.

  • REST end points, which are changed

/market/params => /market/parameters
/oracle/params => /oracle/parameters
/treasury/tax-rate => /treasury/tax_rate
/treasury/tax-rate/{epoch} => /treasury/tax_rate/{epoch}
/treasury/tax-cap => /treasury/tax_cap
/treasury/tax-cap/{denom} => /treasury/tax_cap/{denom}
/treasury/reward-weight => /treasury/reward_weight
/treasury/reward-weight/{epoch} => /treasury/reward_weight/{epoch}
/treasury/tax-proceeds => /treasury/tax_proceeds
/treasury/tax-proceeds/{epoch} => /treasury/tax_proceeds/{epoch}
/treasury/seigniorage-proceeds => /treasury/seigniorage_proceeds
/treasury/seigniorage-proceeds/{epoch} => /treasury/seigniorage_proceeds/{epoch}
/treasury/current-epoch => /treasury/current_epoch
/treasury/params => /treasury/parameters
  • REST end points, which response object key is removed
/treasury/current_epoch
/treasury/seigniorage_proceeds/{epoch}
/treasury/seigniorage_proceeds
/treasury/tax_proceeds/{epoch}
/treasury/tax_proceeds
/treasury/reward_weight/{epoch}
/treasury/reward_weight
/treasury/tax_cap/{denom}
/treasury/tax_rate/{epoch}
/treasury/tax_rate
/oracle/denoms/actives
/oracle/denoms/{denom}/price
/oracle/denoms/{denom}/prevotes/{voter}
/oracle/denoms/{denom}/prevotes
/oracle/denoms/{denom}/votes/{voter}
/oracle/denoms/{denom}/votes
  • New REST endpoints
/supply/total
/supply/total/{denomination}
/market/last_day_issuance
/oracle/voters/{%s}/voting_info
/oracle/voting_infos
/treasury/historical_issuance/{epoch}
  • GradedVestingAccount is fully removed
  • LazyGradedVestingAccount's vesting_lazy_schedules is changed to vesting_schedules
  • Implement issue [FEATURE] Align the UX of fees(gas) and tax(stability)  #200. The sender have to specify fees containing tax amount with following methods.
    1. Use /bank/accounts/{address}/transfers without fees. terracli will compute tax amount and fill fees field containing both gas & tax fee.
    2. Use /txs/estimate_fee to estimate fees of StdTX, and replace StdTx.Fee.Amount to estimated fee
    3. Compute tax with /treasury/tax_rate & /treasury/tax_cap add computed tax with original gas fee

This resolves #219, resolves #175, resolves #195, resolves #197, resolves #200, resolves #209, and resolves #212

Report of required housekeeping

  • Github issue OR spec proposal link
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]

(FOR ADMIN) Before merging

  • Added appropriate labels to PR
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
  • Confirm added tests are consistent with the intended behavior of changes
  • Ensure all tests pass

@yun-yeo yun-yeo requested a review from dokwon September 3, 2019 05:38
@yun-yeo yun-yeo changed the title update to cosmos-v0.37.0 Update to cosmos-v0.37.0 Sep 3, 2019
@yun-yeo yun-yeo force-pushed the feature/update-cosmos-sdk-v0.37.0 branch from 530c0c9 to 4bab8f7 Compare September 3, 2019 05:48
@yun-yeo yun-yeo added client rest / cli code improvements daemon daemon updates enhancement New feature or request must Mustfix for target release. labels Sep 3, 2019
@yun-yeo yun-yeo self-assigned this Sep 3, 2019
@codecov
Copy link

codecov bot commented Sep 3, 2019

Codecov Report

Merging #231 into develop will increase coverage by 1.95%.
The diff coverage is 74.48%.

@@            Coverage Diff             @@
##           develop    #231      +/-   ##
==========================================
+ Coverage    65.74%   67.7%   +1.95%     
==========================================
  Files          105      68      -37     
  Lines         5958    2966    -2992     
==========================================
- Hits          3917    2008    -1909     
+ Misses        1799     849     -950     
+ Partials       242     109     -133

@yun-yeo yun-yeo force-pushed the feature/update-cosmos-sdk-v0.37.0 branch from 7dffed5 to 71e2f78 Compare September 3, 2019 08:53
Copy link
Contributor

@dokwon dokwon left a comment

Choose a reason for hiding this comment

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

Mainly stylistic feedback; did not review market / oracle given lots of the code will change.

tmtypes "github.com/tendermint/tendermint/types"

"github.com/terra-project/core/x/slashing"
"github.com/terra-project/core/x/staking"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Comments for exported functions should start with the function name

- Test state that is needed by `tx` and `query` commands (`home`, `chain_id`, etc...) is stored on the `Fixtures` object. This makes constructing your new tests almost trivial.
- Sometimes if you exit a test early there can be still running `terrad` and `terracli` processes that will interrupt subsequent runs. Still running `terracli` processes will block access to the keybase while still running `terrad` processes will block ports and prevent new tests from spinning up. You can ensure new tests spin up clean by running `pkill -9 terrad && pkill -9 terracli` before each test run.
- Most `query` and `tx` commands take a variadic `flags` argument. This pattern allows for the creation of a general function which is easily modified by adding flags. See the `TxSend` function and its use for a good example.
- `Tx*` functions follow a general pattern and return `(success bool, stdout string, stderr string)`. This allows for easy testing of multiple different flag configurations. See `TestGaiaCLICreateValidator` or `TestGaiaCLISubmitProposal` for a good example of the pattern.
Copy link
Contributor

Choose a reason for hiding this comment

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

TestGaiaCLICreateValidator --> TestTerraCLICreateValidator, TestGaiaCLISubmitProposal --> TestTerraCLISubmitProposal

@@ -0,0 +1,51 @@
# Gaia CLI Integration tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Terra CLI Integration tests

"github.com/cosmos/cosmos-sdk/x/staking"
authutils "github.com/terra-project/core/x/auth/client/utils"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Long file; add short comment on how the test helpers are used

rootCmd.AddCommand(version.VersionCmd)
rootCmd.AddCommand(genutilcli.InitCmd(ctx, cdc, app.ModuleBasics, app.DefaultNodeHome))
rootCmd.AddCommand(genutilcli.CollectGenTxsCmd(ctx, cdc, genaccounts.AppModuleBasic{}, app.DefaultNodeHome))
// terra does not need to support migrage
Copy link
Contributor

Choose a reason for hiding this comment

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

migrage --> migrate.

Why does terra not support this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend removing dead code altogether


cmd.Flags().Float64(client.FlagGasAdjustment, client.DefaultGasAdjustment, "adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored ")
cmd.Flags().String(client.FlagGasPrices, "", "Gas prices to determine the transaction fee (e.g. 10uluna)")
// cmd.MarkFlagRequired(client.FlagGasAdjustment)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove dead code

"github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/cosmos/cosmos-sdk/x/simulation"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment for purpose of file

)

// SimulateDeductFee
func SimulateDeductFee(ak auth.AccountKeeper, supplyKeeper types.SupplyKeeper) simulation.Operation {
Copy link
Contributor

Choose a reason for hiding this comment

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

Strengthen comment to add detail on exported function

return
}

// update luna issuance at last block of a day
Copy link
Contributor

Choose a reason for hiding this comment

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

Updatelastdayissuance --> updatePrevDayIssuance

x/market/abci.go Outdated
"github.com/terra-project/core/x/market/internal/types"
)

// market end block functionality
Copy link
Contributor

Choose a reason for hiding this comment

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

Endblocker is called at the end of every block.

@yun-yeo yun-yeo force-pushed the feature/update-cosmos-sdk-v0.37.0 branch 4 times, most recently from 063e008 to 9e1e284 Compare September 4, 2019 05:04
@yun-yeo yun-yeo requested a review from dokwon September 4, 2019 05:39
@yun-yeo yun-yeo force-pushed the feature/update-cosmos-sdk-v0.37.0 branch from c6f4f8b to 23ea272 Compare September 4, 2019 06:23
Copy link
Contributor

@dokwon dokwon left a comment

Choose a reason for hiding this comment

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

LGTM

@dokwon dokwon merged commit 2929e9f into develop Sep 17, 2019
@dokwon dokwon deleted the feature/update-cosmos-sdk-v0.37.0 branch September 17, 2019 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment