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

refactor: Decouple RainbowConfig #412

Merged
merged 18 commits into from
Nov 16, 2020
Merged

Conversation

frol
Copy link
Contributor

@frol frol commented Nov 5, 2020

Resolves #344

TODO:

  • Avoid direct reading of the config on the spot (pass all the necessary configuration options throgh the arguments)
    • Avoid reading constant config variables (update the interfaces)
    • Avoid readng config variables constructed at runtime (e.g. eth-{tokenName}-address)
    • Test that all the invocations are passing the necessary parameters
  • Avoid direct writes to the config (return value from execute should be treated as diff to the RainbowConfig)

@frol frol force-pushed the refactor/decouple-rainbow-config branch from c2d250b to ca08ea4 Compare November 11, 2020 16:11
@frol frol marked this pull request as ready for review November 12, 2020 16:55
@frol frol requested review from Kouprin and mfornet November 12, 2020 16:55
@frol
Copy link
Contributor Author

frol commented Nov 12, 2020

FINALLY all the tests are green again!

@frol
Copy link
Contributor Author

frol commented Nov 16, 2020

@Kouprin @mfornet Please, review

Copy link
Contributor

@Kouprin Kouprin left a comment

Choose a reason for hiding this comment

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

LGTM. Are there next steps after decoupling?

utils/config/index.js Outdated Show resolved Hide resolved
@Kouprin
Copy link
Contributor

Kouprin commented Nov 16, 2020

CI tests are flaky, I've rebuilt ethrelay catchup one.

@Kouprin
Copy link
Contributor

Kouprin commented Nov 16, 2020

@frol couldn't find an update at testing/adapter/index.js. Is it included?

@frol
Copy link
Contributor Author

frol commented Nov 16, 2020

couldn't find an update at testing/adapter/index.js. Is it included?

@Kouprin It slipped through me when I merged master to my branch. I will see how it fits the new design. Do you think it should be part of the bridge CLI or should be a separate "executable"?

@Kouprin
Copy link
Contributor

Kouprin commented Nov 16, 2020

couldn't find an update at testing/adapter/index.js. Is it included?

@Kouprin It slipped through me when I merged master to my branch. I will see how it fits the new design. Do you think it should be part of the bridge CLI or should be a separate "executable"?

Both are okay. I think it's a question of our understanding who is the clients of CLI. Adapter is built for testing purposes only.

As adapter asks values from config that are changing in runtime, it's impossible to just pass the args. I guess putting it into CLI in preferable (as CLI is the only source for now who handles runtime-changing config values).

@Kouprin
Copy link
Contributor

Kouprin commented Nov 16, 2020

@frol Please let me know your thoughts. I can move adapter into CLI in a separate commit, if we will decide to go this way.

@frol frol merged commit cf20de6 into master Nov 16, 2020
@Kouprin Kouprin deleted the refactor/decouple-rainbow-config branch November 16, 2020 14:39
karim-en pushed a commit that referenced this pull request Dec 20, 2021
# Testing Plan

* [x] The existing end-to-end testing suite pass
* [x] Manual testing of README pass
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.

Rainbow Config refactoring
2 participants