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

client: reduce the amount of magic #560

Closed
ebuchman opened this issue Mar 4, 2018 · 7 comments
Closed

client: reduce the amount of magic #560

ebuchman opened this issue Mar 4, 2018 · 7 comments
Assignees
Labels

Comments

@ebuchman
Copy link
Member

ebuchman commented Mar 4, 2018

Using cobra and viper makes for a lot of magic that can be hard to follow.

It would be nice if functions came with more clarity about what kind of global/external state they are accessing.

Only a very few/small number of functions should utilize global/external state and should be well documented to be doing so.

@rigelrozanski
Copy link
Contributor

Having magic is important is some regard to help offload logic from the new application developers side. The whole paradigm of client is that is can wrap transaction/query commands to add viper, and then also take care of handling the logic associated with those flags that it has created - This is positive as much of this logic with come totally standard many transactions and should not need to be explicitly defined by a new application. Right now I've taken on a few additions to the existing client structure which was (I reckon) originally imported from SDK1 - I haven't really done any major wholistic refactors of client to date - totally agree one is necessary, although I'm not sure I'd like to limit the "magic" (I guess it depends on what you specifically mean). Broadly speaking I would proposed to reorganize the functionality to group:

  • Flag wrapping and associated magical functions for Standard Tx
  • Flag wrapping and associated magical functions for Query
  • Key logic (maybe taking out any viper references??)
  • Custom fully formed transactions (currently the tx folder)

^ probably missing some stuff this is just off the top of my head - thoughts? I can take this on this week

@rigelrozanski rigelrozanski self-assigned this Mar 4, 2018
@ebuchman
Copy link
Member Author

ebuchman commented Mar 4, 2018

Yes I'm interested in explicitly clarifying where calls to global assets are happening. Just want the code to be easier to grok and reason about, which it currently is not with the global viper/cobra calls everywhere

@rigelrozanski
Copy link
Contributor

Here is an idea - let's remove global viper calls all together - why we even need them?!

@rigelrozanski
Copy link
Contributor

Note this is blocking on #544

@rigelrozanski rigelrozanski added the S:blocked Status: Blocked label Mar 9, 2018
@ebuchman
Copy link
Member Author

This is the only kind of magic I want to keep around: https://www.youtube.com/watch?v=UqyT8IEBkvY

@ebuchman ebuchman self-assigned this Mar 13, 2018
@ebuchman ebuchman added lcd C:CLI and removed S:blocked Status: Blocked labels Mar 18, 2018
@ebuchman
Copy link
Member Author

Tiny amount of work done towards this in #624

Still needs lots more

This was referenced Mar 18, 2018
@ebuchman
Copy link
Member Author

Replaced by #721

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants