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

Estimate redemption transaction fee during proposal #3651

Merged
merged 5 commits into from
Jun 28, 2023
Merged

Conversation

lukasz-zimnoch
Copy link
Member

@lukasz-zimnoch lukasz-zimnoch commented Jun 27, 2023

Refs #3614

Here we add a function allowing us to estimate the redemption transaction total fee (coordinator.EstimateRedemptionFee) and we integrate it with the existing coordinator.ProposeRedemption function.

@lukasz-zimnoch lukasz-zimnoch requested a review from pdyraga June 27, 2023 13:37
@lukasz-zimnoch lukasz-zimnoch marked this pull request as ready for review June 27, 2023 13:37
Copy link
Member

@pdyraga pdyraga left a comment

Choose a reason for hiding this comment

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

Just one comment. Other than that, looks good.

case bitcoin.P2WSHScript:
sizeEstimator.AddScriptHashOutputs(1, true)
default:
panic("non-standard redeemer output script type")
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if we should trust the contract validation that much. We can have an upgrade and change the on-chain logic. I think it is OK to assume the script has to be one of the four types listed before but if it is not I would rather return an error, log it, and decide not to propose a redemption instead of killing the process.

At some point in the future

Copy link
Member Author

@lukasz-zimnoch lukasz-zimnoch Jun 28, 2023

Choose a reason for hiding this comment

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

The main purpose of this panic is to signal that something changed on the contract side and requires our attention. I'm afraid that returning an error and just logging it may not be visible enough. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I think the client should panic only if there is no way out. Here there is a way out - we can decide not to propose a redemption and eventually, we can even skip the given script when proposing the redemption. Agreed about calling the attention but I would address it somewhere else, e.g. have a script type for every redemption request at tbtcscan.com

Copy link
Member Author

Choose a reason for hiding this comment

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

See dc866e4

@pdyraga pdyraga enabled auto-merge June 28, 2023 12:30
@pdyraga pdyraga merged commit 898dc9e into main Jun 28, 2023
@pdyraga pdyraga deleted the redemption-fee branch June 28, 2023 12:50
@pdyraga pdyraga added this to the v2.0.0-m4 milestone Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants