-
Notifications
You must be signed in to change notification settings - Fork 37
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
DRAFT: Option to print BTC transactions #176
DRAFT: Option to print BTC transactions #176
Conversation
CI is broken @webwarrior-ws . Moving forward please don't create a PR yet if CI on your fork is not green yet. |
<Reference Include="SharpRaven"> | ||
<HintPath>..\..\packages\SharpRaven.2.4.0\lib\net471\SharpRaven.dll</HintPath> | ||
</Reference> | ||
<Reference Include="Microsoft.CSharp" /> | ||
<Reference Include="System.Configuration" /> | ||
<Reference Include="System.Net.Http" /> | ||
<!-- FIXME: we should not need to reference ResultUtils at all when we can upgrade to F#v4.5 across the board (all CI lanes) --> | ||
<Reference Include="ResultUtils"> | ||
<HintPath>..\..\packages\DotNetLightning.Kiss.1.1.2-date20220322-1031-git-5379324\lib\netstandard2.0\ResultUtils.dll</HintPath> |
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.
@webwarrior-ws you're not using DNL in this PR so why do you add this? we told you to remove unnecessary changes from the PR please, review the PR changes yourself before telling us that you finished
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.
Removed reference to ResultUtils
@@ -111,12 +137,89 @@ type public ElectrumServerReturningInternalErrorException(message: string, code: | |||
originalRequest: string, originalResponse: string) = | |||
inherit ElectrumServerReturningErrorException(message, code, originalRequest, originalResponse) | |||
|
|||
type StratumClient (jsonRpcClient: JsonRpcTcpClient) = | |||
module StratumRequestSerializer = |
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.
@webwarrior-ws same thing with this change, which seems to be a refactoring done in the LN branch (08fc3e5) but not needed for this PR because this PR doesn't have tests
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.
But the module is used in BlockchainScriptHashHistory
and BlockchainTransactionGetVerbose
methods, which were not present before and so don't have "old" versions.
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.
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.
@webwarrior-ws then convert the BlockchainScriptHashHistory and BlockchainTransactionGetVerbose methods into old ones, it's very very very simple and the diff for the PR would be much smaller
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.
Converted the methods to "old style" and minimized changes in StratumClient
<package id="FSharp.Core" version="4.7.0" targetFramework="net471" /> | ||
<package id="Microsoft.Bcl.AsyncInterfaces" version="5.0.0" targetFramework="net472" /> | ||
<package id="Microsoft.Extensions.Logging.Abstractions" version="1.0.2" targetFramework="net471" /> | ||
<package id="NBitcoin" version="6.0.17" targetFramework="net471" /> |
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.
@webwarrior-ws this PR shouldn't need Frontend.Console adding NBitcoin as a dependency. I know you can figure out how to prevent this.
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.
Moved all logic to backend and removed all added dependencies.
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.
Moved all logic to backend and removed all added dependencies.
Thanks. Also can you rebase this PR to get CI green please.
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.
Done. CI is green.
|
||
if not (Seq.isEmpty incomingOutputs) then | ||
let amount = incomingOutputs |> Seq.sumBy (fun txOut -> txOut.Value) | ||
let dateTime = DateTime(1970, 1, 1) + TimeSpan.FromSeconds(float transaction.Time) |
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.
@webwarrior-ws what's this magic number?
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.
Start of UNIX epoch
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 know that; it's my way to say you have to create a variable called unixEpoch, this is programming 101
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.
done
let bitcoinAmount = amount.ToDecimal(NBitcoin.MoneyUnit.BTC) | ||
Console.WriteLine(SPrintF1 "~USD amount: %s" ((bitcoinAmount * price).ToString("F2"))) | ||
| None -> | ||
Console.WriteLine("Could not get bitcoin price for the date") |
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.
@webwarrior-ws no more explanation? if this happens to any user, he would file a bug
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.
Now the program prints exception that occurred when fetching bitcoin price
@@ -480,5 +480,7 @@ let main argv = | |||
UpdateServersFile() | |||
| 1 when argv.[0] = "--update-servers-stats" -> | |||
UpdateServersStats() | |||
| 2 when argv.[0] = "--print-transactions" -> |
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.
@webwarrior-ws let's tweak this feature, if there's a number after the flag --print-transactions
then you would only print that number of transactions (e.g. --print-transactions 3 would only print the last 3 transactions instead of continuing to print all).
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.
done
open GWallet.Backend.UtxoCoin | ||
|
||
|
||
module BTCTransactionPrinting = |
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.
@webwarrior-ws please rename to BtcTransactionPrinting and move it to UtxoCoin folder
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.
done
CI failure on Windows seems to be completely unrelated |
Fixed indentation in .fsproj file |
fc34810
to
a887996
Compare
@webwarrior-ws 2 last things to do here (while you're waiting for CI on the other task):
|
Blockstream electrum servers don't support sending verbose transactions (see [1]), and we use this functionality to get confirmations of a transaction; so we prevent a crash by blacklisting them. [1] Blockstream/electrs#36
I gave this a go today, I found 2 issues @webwarrior-ws , see this example output (where I cut addresses with
The two issues are:
@webwarrior-ws just a reminder, this is a low-priority task, you should only work on this when you're waiting for CI for your high-priority task. Thanks Taras, good progress so far. |
Added --print-transactions option that for given Bitcoin address prints all incoming transactions in reverse chronological order. Data printed: BTC amount, USD amount, date.
When called with --print-transactions and number of transactions is specified, if there are outputs shared between all transactions, print amount and percentage for each such output in each transaction.
1a3be72
to
265b103
Compare
b841302
to
ccb1641
Compare
Superseded by #295 |
Added
--print-transactions
option that for given Bitcoinaddress prints all incoming transactions in reverse
chronological order.
Data printed: BTC amount, USD amount, date.