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

feat(genesis): deployerAddress passed as parameter #2986

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Villaquiranm
Copy link
Contributor

@Villaquiranm Villaquiranm commented Oct 19, 2024

closes #2573

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests

@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Oct 19, 2024
Copy link

codecov bot commented Oct 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.37%. Comparing base (8ec556e) to head (fe837a4).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2986      +/-   ##
==========================================
+ Coverage   63.30%   63.37%   +0.06%     
==========================================
  Files         548      548              
  Lines       78511    78715     +204     
==========================================
+ Hits        49704    49885     +181     
- Misses      25452    25474      +22     
- Partials     3355     3356       +1     
Flag Coverage Δ
contribs/gnodev 61.11% <ø> (+0.53%) ⬆️
contribs/gnofaucet 14.82% <ø> (ø)
gno.land 67.35% <ø> (+0.17%) ⬆️
gnovm 67.88% <ø> (ø)
misc/genstd 79.72% <ø> (ø)
tm2 62.33% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Villaquiranm Villaquiranm force-pushed the feat/configure-address-deployer-genesis branch from 2c5540c to c19da20 Compare October 19, 2024 21:23
@Villaquiranm Villaquiranm changed the title feat(genesis): deployerAddres passed as parameter feat(genesis): deployerAddress passed as parameter Oct 19, 2024
@omarsy omarsy force-pushed the feat/configure-address-deployer-genesis branch from c19da20 to 2d8ba6c Compare October 19, 2024 21:29
@Villaquiranm Villaquiranm force-pushed the feat/configure-address-deployer-genesis branch from 2d8ba6c to c19da20 Compare October 19, 2024 21:30
@Villaquiranm Villaquiranm marked this pull request as ready for review October 20, 2024 07:34
@Villaquiranm
Copy link
Contributor Author

closes #2573

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Depends on #2824

@jefft0 jefft0 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 21, 2024
@thehowl
Copy link
Member

thehowl commented Oct 30, 2024

No longer blocked; please merge master to fix the go.mod CI and fix lint CI. I'll wait for the review team to do a first pass.

// Load Genesis TXs
genesisTxs, err := gnoland.LoadGenesisTxsFile(c.genesisTxsFile, c.chainID, c.genesisRemote)
if err != nil {
return fmt.Errorf("unable to load genesis txs file: %w", err)
}

signer := genesisTxs[0].Msgs[0].GetSigners()[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

the first tx is not necessarily what has been passed to TxsAddPackages command

also loading the examples here will be removed from what I understand so maybe we should not change his file until #1952 is fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the review @n0izn0iz changed back while waiting for that change :)

@jefft0
Copy link
Contributor

jefft0 commented Nov 5, 2024

Hello @Villaquiranm . This PR closes issue #2573 . I see that it adds a way to configure the deployer address from the command line. But the issue also says "". But the issue also says to change to a default "deployer address like g1sh0uldn0thappen..." Is this still needed?

@Villaquiranm
Copy link
Contributor Author

Hey @jefft0 thanks for your review.
I mean I could put g1sh0uldn0thappen but that being a non valid address, maybe it is better to fail if the parameter is not given ? that's why I left it as it was but sure I could put 'g1sh0uldn0thappen'

Hello @Villaquiranm . This PR closes issue #2573 . I see that it adds a way to configure the deployer address from the command line. But the issue also says "". But the issue also says to change to a default "deployer address like g1sh0uldn0thappen..." Is this still needed?

@jefft0
Copy link
Contributor

jefft0 commented Nov 6, 2024

Hey @jefft0 thanks for your review. I mean I could put g1sh0uldn0thappen but that being a non valid address, maybe it is better to fail if the parameter is not given ? that's why I left it as it was but sure I could put 'g1sh0uldn0thappen'

Hello @Villaquiranm . This PR closes issue #2573 . I see that it adds a way to configure the deployer address from the command line. But the issue also says "". But the issue also says to change to a default "deployer address like g1sh0uldn0thappen..." Is this still needed?

Hi @Villaquiranm . I can't tell you the answer. My job as part of "review/triage" is to anticipate the questions that the core devs would ask during their final review, and to make sure they have been considered.

The mnemonic (and private key) of address g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5 is well-known, and thehowl suggests that it shouldn't be the default (like it is in your PR). What do you think are the options to deal with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related review/triage-pending PRs opened by external contributors that are waiting for the 1st review
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

change default genesis package creator, and add way to configure it
5 participants