-
Notifications
You must be signed in to change notification settings - Fork 932
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(rpc): Transaction proxy for staking tx types #984
feat(rpc): Transaction proxy for staking tx types #984
Conversation
Codecov Report
@@ Coverage Diff @@
## main #984 +/- ##
==========================================
- Coverage 58.53% 57.13% -1.41%
==========================================
Files 132 132
Lines 7947 8161 +214
==========================================
+ Hits 4652 4663 +11
- Misses 2818 3023 +205
+ Partials 477 475 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Biggest open question now is the best way to test these! |
0a3c904
to
dd6ca00
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@distractedm1nd did you manually test these endpoints by chance? |
I did not, I need to set up a better mock environment/local dev network, which I have been putting off... How difficult will it be to write unit tests for these, since we are actually depending on app here? Can we mock it easily? |
We unfortunately cannot write unit tests for app-related state interaction right now because of how difficult it is to mock away the app. |
All new endpoints have been tested now and work as expected |
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No comments on the code itself. One thing that I'm not a fan of is that this seems to require/assume that there's a wallet, which has nothing to do with a Celestia Node. Is there some plan to separate the wallet from the node?
@adlerjohn we already have basic "key management" via the cosmos-sdk key utility (see Does that make sense? |
@adlerjohn +1 on what Rene said. Also note that there is no other gateway in our network to request state in trust minimised way besides the node. Here, we just add more state transactions/writes capabilities and further, reads for this state modules will come as well. I am also a big fan of having wallet capabilities on the Node. We already do that to manage balances and further the will be support for addresses/account management. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* staking tx types in `core_access.go` * extending Accessor interface with new staking tx types * adding transaction types to rpc * docs: adding punctuation, removing todo * fix: setting bech32 prefix for validators inside node, new undelegation request types * Update service/rpc/state.go Co-authored-by: rene <41963722+renaynay@users.noreply.github.com> Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
* staking tx types in `core_access.go` * extending Accessor interface with new staking tx types * adding transaction types to rpc * docs: adding punctuation, removing todo * fix: setting bech32 prefix for validators inside node, new undelegation request types * Update service/rpc/state.go Co-authored-by: rene <41963722+renaynay@users.noreply.github.com> Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
Resolves #946