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

Get transaction's confirmation information (including block timestamp) #513

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robbiehanson
Copy link
Contributor

The OnChainOutgoingPayment has a confirmedAt timestamp:

sealed class OnChainOutgoingPayment : OutgoingPayment() {
    abstract override val id: UUID
    abstract val miningFees: Satoshi
    abstract val channelId: ByteVector32
    abstract val txId: ByteVector32
    abstract override val createdAt: Long
    abstract val confirmedAt: Long?
    abstract val lockedAt: Long?
}

Currently this is being set (in Phoenix) by monitoring the blockchain for unconfirmed tx's, and setting confirmedAt = currentTimestampMillis(). However, the timestamp isn't ideal.

It's usually the case that a user sends an on-chain payment, and then backgrounds the app. (Which means the socket connections are closed on iOS.) Only when the user returns to the app (possibly days later) do we realize that the payment was mined, and we set confirmedAt = currentTimestampMillis(). Which, in this example, is days later than when the tx was actually mined.

I'm proposing we add a method like this:

data class ConfirmationStatus(
    val minedAtBlockHeight: Int,
    val currentConfirmationCount: Int,
    val firstBlock: BlockHeader // includes timestamp of when block was mined
)

suspend fun IElectrumClient.getConfirmationStatus(txId: ByteVector32): ConfirmationStatus?

Using this function, we'll be able to set the confirmedAt property to the timestamp of the block, which would be more accurate.

@robbiehanson robbiehanson requested review from pm47 and dpad85 July 19, 2023 20:46
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Concept ACK, but this will be cleaner to do on top of #512, which offers a direct getHeader() function on the IElectrumClient interface. After #512, we could also remove some duplication by having getConfirmationStatus internally call getConfirmations and simply follow up with getHeader().

My goal is to finalize #512 soon, so we should probably wait for this, even though it's a somewhat large change?

data class ConfirmationStatus(
val minedAtBlockHeight: Int,
val currentConfirmationCount: Int,
val firstBlock: BlockHeader // includes timestamp of when block was mined
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
val firstBlock: BlockHeader // includes timestamp of when block was mined
val includedInBlock: BlockHeader // includes timestamp of when block was mined

@robbiehanson
Copy link
Contributor Author

this will be cleaner to do on top of #512

Sounds good. Let's wait for 512, and then revisit.

@t-bast
Copy link
Member

t-bast commented Sep 28, 2023

@robbiehanson you can probably rebase that and do it more easily now!

@t-bast
Copy link
Member

t-bast commented Sep 18, 2024

@robbiehanson if you want this feature, you can rebase now and we should be able to integrate this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants