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: created config base env file #244

Merged
merged 14 commits into from
Apr 6, 2022
Merged

feat: created config base env file #244

merged 14 commits into from
Apr 6, 2022

Conversation

Zfinix
Copy link
Contributor

@Zfinix Zfinix commented Mar 29, 2022

Closes #238

@fadeev
Copy link
Contributor

fadeev commented Mar 30, 2022

This looks good for a small config, but would it scale? For example, one of the things we'll add is fee configuration (see Keplr for example). It might look something like this:

fees:
  - denom: uosmo
    amount:
      low: 1000
      normal: 3000
      fast: 4000
  - denom: uion
    amount:
      low: 10
      normal: 20
      fast: 40

Here's the amount of configuration Keplr requires:

https://github.com/chainapsis/keplr-example/blob/c3d6b483d74aa1521f13d40b291fd62361b2c518/src/main.js#L28-L113

@Zfinix
Copy link
Contributor Author

Zfinix commented Mar 30, 2022

This looks good for a small config, but would it scale? For example, one of the things we'll add is fee configuration (see Keplr for example). It might look something like this:

fees:
  - denom: uosmo
    amount:
      low: 1000
      normal: 3000
      fast: 4000
  - denom: uion
    amount:
      low: 10
      normal: 20
      fast: 40

Here's the amount of configuration Keplr requires:

https://github.com/chainapsis/keplr-example/blob/c3d6b483d74aa1521f13d40b291fd62361b2c518/src/main.js#L28-L113

I believe it would scale, the problem here is that we don't want to build a parser for that structure and we want to be able to still use --dart-define and also change the env variables using hotreload and hot restart, this is only possible if we write it in dart

starport_template/lib/base_env.dart Outdated Show resolved Hide resolved
starport_template/lib/utils/env_util.dart Outdated Show resolved Hide resolved
starport_template/lib/utils/env_util.dart Outdated Show resolved Hide resolved
starport_template/lib/main.dart Outdated Show resolved Hide resolved
@Zfinix Zfinix requested a review from andrzejchm April 1, 2022 13:23
Copy link
Contributor Author

@Zfinix Zfinix left a comment

Choose a reason for hiding this comment

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

@andrzejchm i also added updates to the docs

Copy link
Contributor

@andrzejchm andrzejchm left a comment

Choose a reason for hiding this comment

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

awesome job @Zfinix ! Its much cleaner now. one comment and we're good to merge


class AppConfig {
AppConfig({
this.lcdUrl = 'http://10.0.2.2',
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 10.0.2.2 - localhost for android emulators
  • lhttp://localhost - ios simulator

you'll have to stick with only 1 version, I suggest going with http://localhost for both LCD and grpc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

@Zfinix
Copy link
Contributor Author

Zfinix commented Apr 5, 2022

This is ready to be merged guys @wal33d006 @andrzejchm

@andrzejchm
Copy link
Contributor

please merge at your convenience as soon as the CI checks pass :)

@Zfinix Zfinix merged commit 8fe02d5 into main Apr 6, 2022
@Zfinix Zfinix deleted the feat/add-config-file branch April 6, 2022 11:20
@Zfinix
Copy link
Contributor Author

Zfinix commented Apr 6, 2022

Merging this now

@Zfinix Zfinix added 📄 Documentation 📄 Improvements or additions to documentation Technical 🛠 Technical task not closely coupled with any feature and removed 🔴 DO NOT MERGE 🔴 labels Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 Documentation 📄 Improvements or additions to documentation Technical 🛠 Technical task not closely coupled with any feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add a config file
4 participants