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

'Type' is the only field in listtransactions that is capitalised #2419

Closed
delta1513 opened this issue Dec 28, 2021 · 7 comments · Fixed by #2441
Closed

'Type' is the only field in listtransactions that is capitalised #2419

delta1513 opened this issue Dec 28, 2021 · 7 comments · Fixed by #2441
Assignees
Milestone

Comments

@delta1513
Copy link

Bug Report

Current behavior

When running listtransactions via the wallet console or RPC, the Type field is capitalised as shown below in an example of one transaction.

{
"account": "",
"address": "mfdCQCGjStwK7G9PdZx5gmnHnS2mmB1Prh",
"category": "generate",
"Type": "POS",               // <-------- BUG ON THIS LINE
"fee": 0.00000000,
"amount": 10.00000000,
"confirmations": 20006,
"generated": true,
"blockhash": "a6cb0a4304fae1fb0fa39a54ca859625081efb92a12111eeba65610d0e25eef3",
"blockindex": 1,
"blocktime": 1638758544,
"txid": "7755386dbb52ce2e3bae66794daec321ff13583a95a78b48b470f7184f7dbe78",
"time": 1638758544,
"timereceived": 1638758545
}

Expected behavior

When running listtransactions via the wallet console or RPC, the Type field should not be capitalised, ie, it should be type.

Steps to reproduce:

Run the listtransactions RPC

Gridcoin version

v5.3.2.0-unk

Machine specs

Irrelevant

@delta1513 delta1513 added the bug label Dec 28, 2021
@delta1513
Copy link
Author

I can see this issue not only affecting internal RPC operations within the wallet but also external operations. Namely my wallet bot and possibly other services.

The reason I bring this up is I think this issue needs to be made as obvious as possible in the changelog for the release this fix come in.

@jamescowens
Copy link
Member

Yes. And it must wait until a mandatory release and be given notice beforehand.

@jamescowens jamescowens added this to the Kermit's Mom milestone Dec 28, 2021
@jamescowens jamescowens self-assigned this Dec 28, 2021
@div72
Copy link
Member

div72 commented Dec 29, 2021

Should be merged with #527 in my opinion.

@jamescowens
Copy link
Member

I partially agree. I am not in favor of as radical a change as #527 advocates, as changing key names introduces unnecessary disruption. As @delta1513 in Discord suggested, this whole thing gets to be a little OCD like.

@div72
Copy link
Member

div72 commented Dec 29, 2021

I vote for pulling the bandage off swiftly in this case. This is a non-brainer for non-Gridcoin-specific RPCs(I haven't verified if any exists, but I assume after not following upstream for a while we're bound to deviate especially for commands with Gridcoin extensions) however it is also better to do it as more time goes on it'll be harder to change as the usage of the RPCs grows by time in my opinion. Code-wise, we'll have to touch every RPC when porting the modern RPC framework from Bitcoin anyways. I personally can do this and provide a script to ease transition if desired.

Another argument for doing this in one go is that changing a single key already causes disruption, and the disruption does not scale linearly with each key changed. I love consistency and I believe it improves both new developers' experience but also future projects of old developers too. (But maybe I just have OCD, and this is a justification. ¯\_(ツ)_/¯ )

@startailcoon @bryhardt @delta1513 @sau412 I would like to hear your opinions as well as you are the people who's going to be most affected by such a change.

@jamescowens
Copy link
Member

jamescowens commented Dec 29, 2021 via email

@sau412
Copy link

sau412 commented Dec 30, 2021

I would like to hear your opinions as well as you are the people who's going to be most affected by such a change.

I checked my code and I'm not using that field name anywhere.

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

Successfully merging a pull request may close this issue.

4 participants