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

WIP: Replace GenesisTx with StdTx #2422

Closed
wants to merge 47 commits into from

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Sep 29, 2018

Ref #1890

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@zmanian
Copy link
Member

zmanian commented Sep 29, 2018

Hey would it be possible think about aligning the requirements here with our needs to Starfish for launch?

Some description of the idea of Starfish.

https://gist.github.com/zmanian/88fdd41421d363d11adbcf98d3367a4c

@codecov
Copy link

codecov bot commented Oct 1, 2018

Codecov Report

Merging #2422 into develop will increase coverage by 3.38%.
The diff coverage is 9.52%.

@@             Coverage Diff             @@
##           develop    #2422      +/-   ##
===========================================
+ Coverage    57.87%   61.25%   +3.38%     
===========================================
  Files          140      122      -18     
  Lines         8119     7519     -600     
===========================================
- Hits          4699     4606      -93     
+ Misses        3123     2594     -529     
- Partials       297      319      +22

@cwgoes
Copy link
Contributor

cwgoes commented Oct 2, 2018

@zaki What will be the process for collecting genesis accounts for the Hub? Will the ICF issue a "suggested" allocation list, which node operators can then combine with their list of genesis transactions locally - or will the ICF collate genesis transactions itself and provide a full genesis file?

@cwgoes
Copy link
Contributor

cwgoes commented Oct 2, 2018

Let's include this in 0.25.

@cwgoes cwgoes mentioned this pull request Oct 2, 2018
23 tasks
@zmanian
Copy link
Member

zmanian commented Oct 3, 2018

It's gonna be more the like here are allocations + genesis txs + tools for reading eth and Bitcoin blockchains.

@jackzampolin
Copy link
Member

Lets make sure we at least have an eye toward starfish requirements while implementing this. Lets sync on this @alessio

@zmanian
Copy link
Member

zmanian commented Oct 6, 2018

I feel like we can implement Starfish as wrapper around this functionality.

@jaekwon
Copy link
Contributor

jaekwon commented Oct 8, 2018

Reviewing...

@jaekwon jaekwon self-requested a review October 8, 2018 00:09
@jaekwon jaekwon self-assigned this Oct 8, 2018
Power: 1,
Name: fmt.Sprintf("validator-%d", i+1),
},
for i:=0 ; i<nValidators; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

dry w/ above

@@ -107,7 +107,7 @@ func PrintUnsignedStdTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, msg

// SignStdTx appends a signature to a StdTx and returns a copy of a it. If appendSig
// is false, it replaces the signatures already attached with the new signature.
func SignStdTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string, stdTx auth.StdTx, appendSig bool) (auth.StdTx, error) {
func SignStdTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string, stdTx auth.StdTx, appendSig bool, offline bool) (auth.StdTx, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally is maybe SignStdTxOptions struct/ptr? But this is maybe fine for now w/ comment & TODO in comments in code... (for me anyways)

if len(req.Validators) > 0 {
if len(req.Validators) != len(validators) {
panic(fmt.Errorf("len(RequestInitChain.Validators) != len(validators) (%d != %d) ", len(req.Validators), len(validators)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO sort and equality check...

@@ -32,6 +31,7 @@ var (
// State to Unmarshal
type GenesisState struct {
Accounts []GenesisAccount `json:"accounts"`
Txs []json.RawMessage `json:"txs"`
Copy link
Contributor

Choose a reason for hiding this comment

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

[]StdTx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was the original implementation. I could roll it back, though this is not blocking now


if initConfig.GenTxs {
validators, appGenTxs, persistentPeers, err = processGenTxs(initConfig.GenTxsDir, cdc)
if withGenTxs {
Copy link
Contributor

Choose a reason for hiding this comment

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

block comment here on what withGenTxs does, namely aggregates gentxs.

if err != nil {
return
}
var genTx server.GenesisTx
err = cdc.UnmarshalJSON(bz, &genTx)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

block comment on the alternative logic, which just creates a simple genesis file with no gentxs

return cmd
}

func initWithConfig(cdc *codec.Codec, appInit server.AppInit, config *cfg.Config, initConfig server.InitConfig) (
chainID string, nodeID string, appMessage json.RawMessage, err error) {
func initWithConfig(cdc *codec.Codec, appInit server.AppInit, config *cfg.Config, chainID, moniker, genTxsDir string, withGenTxs, overwriteGenesis bool) (
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO document what this function does, including side effects like possibly creating priv validator file... etc

@@ -273,3 +206,73 @@ func GaiaAppGenStateJSON(cdc *codec.Codec, appGenTxs []json.RawMessage) (appStat
appState, err = codec.MarshalJSONIndent(cdc, genesisState)
return
}


// ProcessStdTxs processes and validates application's genesis StdTxs and returns the list of validators,
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ProcessStdTxs/CollectGenTxs/g

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

err = errors.New("must provide at least genesis message")
return
}
msg := msgs[0].(stake.MsgCreateValidator)

// create the genesis account, give'm few steaks and a buncha token with there name
Copy link
Contributor

Choose a reason for hiding this comment

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

"their name" not "there name"... some elses typo

stakeData := stake.DefaultGenesisState()
validator := stake.NewValidator(sdk.ValAddress(acc.Address), pubKey, stake.NewDescription(moniker, "", "", ""))
stakeData = addValidatorToStakeData(validator, stakeData)
stakeData.Pool.LooseTokens = stakeData.Pool.LooseTokens.Add(sdk.NewDecFromInt(freeFermionsAcc))
Copy link
Contributor

Choose a reason for hiding this comment

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

just reading the code, it looks like it's being done twice here? once in the function addValdiatorToStakeData above.

@alessio alessio force-pushed the alessio/1890-replace-genesistx-with-stdtx branch from 7360d59 to 4c85964 Compare October 17, 2018 22:19
@alessio
Copy link
Contributor Author

alessio commented Oct 18, 2018

Superseded, see #2524

@alessio alessio closed this Oct 18, 2018
@alessio alessio deleted the alessio/1890-replace-genesistx-with-stdtx branch October 18, 2018 23:43
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.

5 participants