Fabo/sdk 0.26.1-rc1 #1515
Fabo/sdk 0.26.1-rc1 #1515
Conversation
|
Waiting on #1512 |
|
@faboweb |
| @@ -28,9 +23,7 @@ const Client = (axios, localLcdURL, remoteLcdURL) => { | |||
| if (Array.isArray(args)) { | |||
| args = args.join(`/`) | |||
| } | |||
| if (method === `DELETE`) { | |||
There was a problem hiding this comment.
I remember that the DELETE requests needed to have this nested data structure. Did you test if any DELETE requests works like this?
There was a problem hiding this comment.
No, I tried to but I'm unable to start Voyager on this branch.
I deleted the above code because the behavior changed when I called Axios using axios({method: 'delete', ...}) instead of axios.delete(... and this change was necessary to make the tests pass again. At least one of the tests was the contract test keys.delete against a real HTTP server so it's likely that any other delete requests will work this way as well.
There was a problem hiding this comment.
cool, just wanted to make sure
|
@faboweb lint is failing |
|
@fedekunze still a lot failing ;) |
Co-Authored-By: faboweb <[email protected]>
|
@NodeGuy the suggestion made the linter cry. Could you reformat and push if you find time? |
| } | ||
|
|
||
| // execute command and return stdout | ||
| function makeExec(command) { |
There was a problem hiding this comment.
If you use const makeExec = command => instead then it won't get hoisted and I would have spent less time looking for it.
There was a problem hiding this comment.
I like hoisting with helper functions. Keeps the top of the file for the important functions. But I understand your approach.
Co-Authored-By: faboweb <[email protected]>
| ` --commission-max-rate=0` + | ||
| ` --commission-rate=0` + | ||
| ` --json` | ||
|
|
There was a problem hiding this comment.
let command = `${cliBinary} tx create-validator
--home ${clientHomeDir}
--from ${keyName}
--amount=10steak
--pubkey=${valPubKey}
--address-delegator=${operatorAddress}
--moniker=${moniker}
--chain-id=test_chain
--commission-max-change-rate=0
--commission-max-rate=0
--commission-rate=0
--json`There was a problem hiding this comment.
this command includes several \n which doesn't work
| value = parseInt(value.trim().substr(0, value.length - 2)) | ||
| } else if (value.trim().endsWith(`s`)) { | ||
| value = parseInt(value.trim().substr(0, value.length - 1)) * 1000 | ||
| } |
There was a problem hiding this comment.
It would be good to put this parsing code into a separate function and write unit tests for it.
There was a problem hiding this comment.
This feels excessive for a one shot. This is also implicitly tested by running this as part of the e2e test. Do you disagree?
| on: () => {}, | ||
| pipe: () => {} | ||
| kill: () => {}, | ||
| removeAllListeners: () => {}, |
There was a problem hiding this comment.
Why are you implementing removeAllListeners instead of letting EventEmitter do it for you?
There was a problem hiding this comment.
using EventEmitter produced side effects. and I prefer opting in to features so I know what to expect from a mock. do you disagree?
Which suggestion? |
Co-Authored-By: faboweb <[email protected]>
Nevermind, did it. |
| @@ -3,7 +3,9 @@ | |||
| const Client = (axios, localLcdURL, remoteLcdURL) => { | |||
| async function request(method, path, data, useRemote) { | |||
| const url = useRemote ? remoteLcdURL : localLcdURL | |||
| return (await axios({ data, method, url: url + path })).data | |||
| const result = await axios({ data, method, url: url + path }) | |||
| console.log(method, path, data, result.data) | |||
There was a problem hiding this comment.
delete console log
| console.log(method, path, data, result.data) |
Description:
Current commit:
bb54a0de127e45713f272217f578c0abe53a5b21❤️ Thank you!
CHANGELOG.mdwith issue # and GitHub usernameFiles changedin the github PR explorer