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

Monitor incoming trades #3201

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Abrynos
Copy link
Contributor

@Abrynos Abrynos commented May 4, 2024

Checklist

  • I read and understood the Contributing Guidelines.
  • This is not a duplicate of an existing merge request.
  • I believe this falls into the scope of the project and should be part of the built-in functionality.
  • My code follows the code style of this project.
  • I have added tests to cover my changes, wherever they are necessary.
  • All new and existing tests pass.

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.

@Abrynos Abrynos marked this pull request as draft May 4, 2024 23:24
@Abrynos
Copy link
Contributor Author

Abrynos commented May 4, 2024

Draft, as I am not completely sure about this feature. Waiting for feedback before marking as ready for review.

@Abrynos Abrynos added ✨ Enhancement Issues marked with this label indicate further enhancements to the program, such as new features. 📢 Feedback welcome Issues marked with this label are open to any potential feedback that could help us. 👀 Evaluation Issues marked with this label are currently being evaluated if they're going to be considered. labels May 4, 2024
@JustArchi JustArchi added 🚧 Work in progress Issues marked with this label are in active work-in-progress and they're not ready for review yet. and removed 👀 Evaluation Issues marked with this label are currently being evaluated if they're going to be considered. labels May 5, 2024

switch (result.Result) {
case ParseTradeResult.EResult.Accepted:
++AcceptedOffers;
Copy link
Member

@JustArchi JustArchi May 5, 2024

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.

Copy link
Contributor Author

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.

Copy link
Member

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")
Copy link
Member

@JustArchi JustArchi May 5, 2024

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Enhancement Issues marked with this label indicate further enhancements to the program, such as new features. 📢 Feedback welcome Issues marked with this label are open to any potential feedback that could help us. 🚧 Work in progress Issues marked with this label are in active work-in-progress and they're not ready for review yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants