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
Monitor incoming trades #3201
base: main
Are you sure you want to change the base?
Monitor incoming trades #3201
Conversation
Draft, as I am not completely sure about this feature. Waiting for feedback before marking as ready for review. |
|
||
switch (result.Result) { | ||
case ParseTradeResult.EResult.Accepted: | ||
++AcceptedOffers; |
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.
Use post-increment AcceptedOffers++
if you don't need pre-increment, like here, it's more readable imo. Every single sane compiler optimizes it in the same way since decades.
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.
I write it this way out of habit and to be honest I'm too lazy to change this now. If it's that important to you, I'll do it, but I'd say it's a quite superfluous change.
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.
It's important for me to have consistency across ASF repository, so yeah, it is important to me - but I can do it myself post-PR if needed.
kv.Value.ConfirmedOffers, | ||
new KeyValuePair<string, object?>(TagNames.BotName, kv.Key.BotName), | ||
new KeyValuePair<string, object?>(TagNames.SteamID, kv.Key.SteamID), | ||
new KeyValuePair<string, object?>(TagNames.TradeOfferResult, "2fa_confirmed") |
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.
This is probably misleading, a donation trade offer will be Confirmed
despite no confirmation being needed, even for people without 2FA.
Consider automatically_confirmed
, accepted_and_confirmed
, confirmed
or something similar instead.
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.
I'm not sure what you mean exactly. Can you elaborate?
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.
I'm not sure what you mean exactly. Can you elaborate?
2fa_confirmed
name.
Checklist
Changes
As requested on our community Discord server.
New functionality
Metrics endpoint now provides information about trades received since last ASF restart.
Changed functionality
None
Removed functionality
None
Additional info
I do not want to include too much information (e.g. which items [marketable, rarity, appid, type, assetid, classid, contextid] were traded) due to performance/memory considerations. As soon as we start adding those details, this will quickly get out of hand.
Thank you for considering the inclusion of this merge request.