Config: AssetEnabled upgrade #1735
Conversation
gbjk
force-pushed
the
feature/config_assetenabled
branch
2 times, most recently
from
3697ede to
5fb08d2
Compare
gloriousCode
added
the
nomerge requires dependency
gbjk
force-pushed
the
feature/config_assetenabled
branch
from
026f88f to
272827a
Compare
gbjk
force-pushed
the
feature/config_assetenabled
branch
from
942b49b to
e2d81d0
Compare
gbjk
removed
the
nomerge requires dependency
gbjk
force-pushed
the
feature/config_assetenabled
branch
from
c092bfa to
65c05a0
Compare
Fixed gbjk@65c05a063 |
gbjk
force-pushed
the
feature/config_assetenabled
branch
from
65c05a0 to
4a4fb99
Compare
| if err != nil { | ||
| log.Errorln(log.ExchangeSys, err) | ||
| log.Errorf(log.ExchangeSys, "%s error storing `%s` default asset formats: %s", {{.Variable}}.Name, asset.Spot, err) |
There was a problem hiding this comment.
Can you wrap all these details on the error paths in StoreAssetPairStore? That way we don't need to have this.
There was a problem hiding this comment.
The whole direction here was the opposite of this change request.
The ideology I'm following is that functions return the error they encounter, without their own context.
If you call a go standard library function, you'll get the same thing.
It's down to the caller to decide if it wants to give context to that error, and what that context looks like from it's perspective.
From the perspective of the function caller, they know what they called, and so the error returned should be undecorated.
Case in point: StoreAssetPairStore is not default asset formats so these callers all needed to clarify the context they were calling that function in.
This is why you'll often see me decorate errors one level above where GCT has previously been doing it.
Sometimes it can feel slightly weighty, in this example where we copy the same context everywhere.
We could definitely add a common error for errStoringDefaultAssetFormats though, and I think that's warranted.
I'll circle back to this after the others and see if you've replied :)
There was a problem hiding this comment.
Adding one more bit of context:
This implies that functions must also add their own context.
e.g. If StoreAssetPairStore encountered some error interacting with a database, like "Record not found", We'd expect that error to come without context.
So StoreAssetPairStore should add the context error searching for glove size in database: %w.
There was a problem hiding this comment.
From an implementation perspective, maintaining this standard can introduce toil during implementation and reviews, especially since any failure in SetDefaults would inherently cause the exchange to malfunction. Differentiating between errors in this context seems redundant. Still suggest wrapping asset and exchange for the exchange method.
But I will leave this open for other peoples perspective. @thrasher- @gloriousCode
There was a problem hiding this comment.
Looking at what both of you saying, I don't think there should be a hard rule about either. There will be times I will absolutely argue for errors returned to contain the data which it is complaining about + some extra context tidbits. Things like "they know what they called" in a large, semi-automated system like GCT doesn't work as well as you'd hope and I wouldn't want that universally applied.
Another issue which depends on the situation is the onus on the caller, so for some functions that would mean decorating the returned error in a large amount of areas of code (which you have done in 11 places) could have just been done in the function once.
Like Shazbert says, the onus is then put on the reviewer to say "hey decorate that error" when a new call is added. If everything relies on reviewers catching people not doing things on top of all the things they are doing, then we are going to get unhelpful, undecorated errors. An unlabelled asset is invalid in the logs is useless which is why we do what we do.
Given that this function is only called within SetDefaults() and gk has copy pasted the same message everywhere, I'm okay with it staying - even if I find it strange to advocate for copy and pasting strings in 11 places. You won't be able to use this text to get out of all future instances 😄
There was a problem hiding this comment.
That's an interesting point about how GCT is a system.
I'd be taking a completely different perspective if GCT was more closed, and the paths to these functions were all private, and callers knew to log it without decoration.
Also: logged with double decoration is better than logged with no context. That's an argument against my change.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1735 +/- ##
==========================================
+ Coverage 37.13% 37.38% +0.24%
==========================================
Files 414 416 +2
Lines 180198 178480 -1718
==========================================
- Hits 66925 66723 -202
+ Misses 105413 104001 -1412
+ Partials 7860 7756 -104
|
gbjk
force-pushed
the
feature/config_assetenabled
branch
from
5293a16 to
c1cbeff
Compare
gbjk
force-pushed
the
feature/config_assetenabled
branch
from
c1cbeff to
bfa72e2
Compare
|
I'm going to rebase this onto #1770, because I think that's kinda necessary for |
gbjk
force-pushed
the
feature/config_assetenabled
branch
from
bfa72e2 to
65d4423
Compare
gbjk
added
the
nomerge requires dependency
|
Clarification: 1770 makes this manually testable for Downgrade, which is what caught @gloriousCode out. |
gbjk
removed
the
nomerge requires dependency
gbjk
force-pushed
the
feature/config_assetenabled
branch
from
65d4423 to
79a651c
Compare
| if err != nil { | ||
| log.Errorln(log.ExchangeSys, err) | ||
| log.Errorf(log.ExchangeSys, "%s error storing `%s` default asset formats: %s", {{.Variable}}.Name, asset.Spot, err) |
There was a problem hiding this comment.
Looking at what both of you saying, I don't think there should be a hard rule about either. There will be times I will absolutely argue for errors returned to contain the data which it is complaining about + some extra context tidbits. Things like "they know what they called" in a large, semi-automated system like GCT doesn't work as well as you'd hope and I wouldn't want that universally applied.
Another issue which depends on the situation is the onus on the caller, so for some functions that would mean decorating the returned error in a large amount of areas of code (which you have done in 11 places) could have just been done in the function once.
Like Shazbert says, the onus is then put on the reviewer to say "hey decorate that error" when a new call is added. If everything relies on reviewers catching people not doing things on top of all the things they are doing, then we are going to get unhelpful, undecorated errors. An unlabelled asset is invalid in the logs is useless which is why we do what we do.
Given that this function is only called within SetDefaults() and gk has copy pasted the same message everywhere, I'm okay with it staying - even if I find it strange to advocate for copy and pasting strings in 11 places. You won't be able to use this text to get out of all future instances 😄
It was already rebased for you, when I removed restructuring 😄 At least I hope. |
gbjk
force-pushed
the
feature/config_assetenabled
branch
2 times, most recently
from
497c276 to
1827edb
Compare
gbjk
force-pushed
the
feature/config_assetenabled
branch
from
d1fb399 to
30ca6a9
Compare
This became more messy with Disabling something that's defaulted to disabled. Taking an idealogical stance against erroring that what you want to have done is already done.
gbjk
force-pushed
the
feature/config_assetenabled
branch
2 times, most recently
from
d1fb399 to
91e3344
Compare
Previously we were calling it "Format", but accepting everything from the PairStore. We were also defaulting to turning the Asset on. Now callers need to get their AssetEnabled set as they want it, so there's no magic This change also moves responsibility for error wrapping outside to the caller.
Previously we ignored the field and just turned on everything. I think that was because we couldn't get at the old value. In either case, we have the option to do better, and respect the assetEnabled value
gbjk
force-pushed
the
feature/config_assetenabled
branch
from
b1821d4 to
9d516e9
Compare
Config:
Exchanges:
StoreAssetPairFormattoStoreAssetPairStoreSetAssetEnabledwill not error on no-op (already disabled or enabled)Bitfinex:
Type of change
Please delete options that are not relevant and add an
xin[]as item is complete.How has this been tested