-
Notifications
You must be signed in to change notification settings - Fork 10
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
Implemented date range for getting account transactions #61
Merged
fudgebucket27
merged 8 commits into
fudgebucket27:master
from
modersohn:feature/TransactionCSVExport
Mar 13, 2022
Merged
Implemented date range for getting account transactions #61
fudgebucket27
merged 8 commits into
fudgebucket27:master
from
modersohn:feature/TransactionCSVExport
Mar 13, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* workaround getting first/last block id in the timespan * then using Transaction_filter block_gte/lte * extended GetBlock ** optional params for sorting instead of hardcoded ** optional timestamp gte/lte ** kept changes even though not used because GetBlockDate is more effective getting both block IDs in one go ** also demonstrated building a json request and adding optional parameters to it, by using JObject and then request.AddStringBody ** added tests * new GetBlockDateRange ** returns both block IDs as a tuple ** combined query using alias ** added test with some hard-coded dates/IDs from the past * extended GetAccountTransactions ** finished part where date range is given ** calls GetBlockDateRange and then uses block_gte/lte * renamed/extended TimestampConverter ** ToUTC for string and DateTime ** ToTimeStamp for the otherway around ** added minimal test-case with a roundtrip * CSV export ** disabled CSV type as only one type supported right now ** added work-in-progress warning ** re-enabled date edits ** TransactionExportService now raises exception if no transactions where found (exceptions are already handled by ExportDialog)
* block ID range is now determined only once, not with every reqest to get more transactions * refactored GetAccountTransactions, adapted tests
* added Deposit, Withdrawal and TransferNFT * improved Transfer: checking from/toAccount for accountIdPerspective to determine if we received or removed
* accountIdPerspective must be compared to account.id not address
* since only date is entered, it should be fully inclusive
* fixed typo in DefaultCSVFormat class and file * added registration mechanism ** no auto-registration built-in, so manually register in Program.cs ** dynamic format selection in ExportDialog * refactored various CSV export aspects so new Cointracking can just descend from Default format * added mechanism to modify download filename, so Cointracking can use .csv extension etc. * Cointracking format has quick and dirty quotedString method, which should later be refactored into something generally usable
* for exporting use ToDecimal ** then convert to string without a format ** that always gives the best precision and no thousand delimiters ** consequently use GetExportAmount in one central place * for display use ToString ** but generate format on the fly ** which has thousand separators and *optional* decimal places ** that is only possible via the #,##0.#### format ** so generate it with "token.decimals" ** but reduce for larger amounts, i.e. Log10 to retrieve exponent * adjust tests - no longer enforcing unnecessary decimal places * fix model: up until now double fields are never optional, so they shouldn't be here too
* I have not encountered a transaction where fillBA/B does not equal fillSB/A so I presume this is not necessary * also rather risk it to be wrong than raise an exception, so we can eventually get better user feedback
hey mate finally had a chance to sit down and review. everything looks great and it passes the tests so i am happy with that too. i will merge this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
more effective getting both block IDs in one go
parameters to it, by using JObject and then
request.AddStringBody
where found (exceptions are already handled by ExportDialog)