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

Inventory fetching through CM #3155

Merged
merged 12 commits into from
Mar 16, 2024

Conversation

ezhevita
Copy link
Member

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

New functionality

None

Changed functionality

Inventory fetching now works using CM requests instead of web API, there are no rate-limits and it allows loading even 82k+ inventories in about 11 seconds.

Removed functionality

Fetching foreign inventories is no longer possible, however it wasn’t used in ASF anyway and it is rate-limited to hell ¯\_(ツ)_/¯

Additional info

This code was in production for about a year so it works pretty stable and Steam didn’t break anything in this time period. Also current implementation of AdditionalProperties is pretty awkward so any suggestions to improve it are welcome.

@JustArchi
Copy link
Member

After brief evaluation, good stuff 👍

I'd use this opportunity to refactor and reuse components as much as possible instead of bringing up our own ones:

  • Since this is CM call, you most likely have proto body already - we should use it if possible instead of bringing up our own re-implementations and on top of it exposing additional properties in non-unified way.
  • We can still utilize general composition/wrapper pattern, for example, we can initialize our own class taking protobuf data in constructor, have a readonly field for keeping it, then adding appropriate getters/setters we care about reaching it. Moreover, the field can be public which would allow plugin creators to extract other information from it without our hard mappings.
  • In regards to description, I was thinking about it for a while, and it might make sense to just have description as such field/property inside item, with maybe additional helper properties like uint RealAppID => Description.RealAppID. I don't like our current code that does a lot of back and forth parsing/setting.
  • I'd like to keep method in AWH, because it can still be used for fetching other inventories than our own one, EVEN if heavily limited. We can make new classes described above compatible with it - use protobuf member as "backing" field for data, mark it as [JsonIgnore], then add helper properties with getter and setters, annotate them as json-compatible, and add logic for each property to get/set appropriate value in underlying backing protobuf field. This way you'd have one "superclass" for asset and description, that'd still be compatible with both, being constructed from CM response (you pass received protobuf as constructor), as well as AWH json response (json serializer creates it, then calls setters for each helper property, effectively manipulating underlying protobuf without even realizing).

Does that make sense to you? If yes, we don't need to worry about breaking changes much (for plugin creators), as such refactor is totally justified for future of the program.

@JustArchi JustArchi added ✨ Enhancement Issues marked with this label indicate further enhancements to the program, such as new features. 👀 Evaluation Issues marked with this label are currently being evaluated if they're going to be considered. 🚧 Work in progress Issues marked with this label are in active work-in-progress and they're not ready for review yet. 📢 Feedback welcome Issues marked with this label are open to any potential feedback that could help us. 🔧 Fixes required Issues marked with this label require further follow-up fixes before they can be considered. and removed 👀 Evaluation Issues marked with this label are currently being evaluated if they're going to be considered. labels Mar 10, 2024
…for InventoryDescription, add properties to description
@ezhevita ezhevita marked this pull request as draft March 11, 2024 21:49
Copy link
Member

@JustArchi JustArchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote on Discord, the current direction this PR is going is superb, we only need to fix all pending warnings and other errors - feel free to request re-review after that happens and I'll be happy to merge it in if no outstanding problems are found.

@ezhevita ezhevita marked this pull request as ready for review March 13, 2024 00:23
@JustArchi JustArchi added 🏁 Finished Issues marked with this label were finished already and no further work is required on them. and removed 🔧 Fixes required Issues marked with this label require further follow-up fixes before they can be considered. 🚧 Work in progress Issues marked with this label are in active work-in-progress and they're not ready for review yet. labels Mar 13, 2024
@JustArchi JustArchi requested review from Abrynos and Rudokhvist March 14, 2024 11:27
Copy link
Member

@JustArchi JustArchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly misc stuff, everything else LGTM

ArchiSteamFarm/Steam/Data/Asset.cs Show resolved Hide resolved
@JustArchi JustArchi merged commit 1842329 into JustArchiNET:main Mar 16, 2024
21 checks passed
@JustArchi
Copy link
Member

Thanks a lot! 🏆

@ezhevita ezhevita deleted the new-inventory-fetching branch March 17, 2024 17:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. 🏁 Finished Issues marked with this label were finished already and no further work is required on them.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants