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

feat: tss funds migration #1143

Merged
merged 30 commits into from
Oct 16, 2023
Merged

feat: tss funds migration #1143

merged 30 commits into from
Oct 16, 2023

Conversation

kingpinXD
Copy link
Contributor

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Closes:

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Checklist:

  • I have added unit tests that prove my fix feature works

@kingpinXD kingpinXD changed the title Tss migration cctx Tss funds migration Sep 18, 2023
@kingpinXD kingpinXD changed the title Tss funds migration feat : tss funds migration Sep 19, 2023
@kingpinXD kingpinXD linked an issue Sep 20, 2023 that may be closed by this pull request
@kingpinXD kingpinXD marked this pull request as ready for review October 11, 2023 20:11
@kingpinXD kingpinXD changed the title feat : tss funds migration feat: tss funds migration Oct 11, 2023
x/crosschain/keeper/migrate_tss_funds.go Outdated Show resolved Hide resolved
x/crosschain/keeper/msg_tss_voter.go Outdated Show resolved Hide resolved
x/crosschain/keeper/msg_tss_voter.go Outdated Show resolved Hide resolved
x/crosschain/keeper/msg_tss_voter.go Outdated Show resolved Hide resolved
x/crosschain/types/messages_migrate_tss_funds.go Outdated Show resolved Hide resolved
zetaclient/evm_signer.go Outdated Show resolved Hide resolved
x/crosschain/keeper/migrate_tss_funds.go Outdated Show resolved Hide resolved
x/crosschain/keeper/migrate_tss_funds.go Outdated Show resolved Hide resolved
@kingpinXD kingpinXD requested a review from lumtis October 13, 2023 18:52
estimateFee := float64(gasPrice.Uint64()) * outTxBytesMax / 1e8
if cointype == common.CoinType_Cmd {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for real Tss migration (TSS had 3 hundreds of UTXOs in Athens3), if we pays only outTxBytesMin of fee, it may take many blocks (10min per block) for the outTx (many many inputs) to be accepted in a block.

For TSS migration, the thing is to make sure the old tss address has enough funds >= 80_000 sats (4000 * gasPrice (e.g., 20)) to move its own funds away.

Also, I believe the hard coded estimateFee = 4000 can be improved in future too, but that only helps in cases when TSS has only $10 (for example) of BTC in reserve and we want to move $5 to new TSS and paying another $5 as gas fee.

To conclude, we can just remove the estimateFee adjustment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand completely, you mean to remove the estimate fee component completely?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think comment out line 74 and leave the calculation estimateFee := float64(gasPrice.Uint64()) * outTxBytesMax / 1e8 as is would be enough.

@github-actions
Copy link

!!!WARNING!!!
nosec detected in the following files: zetaclient/bitcoin_client.go

Be very careful about using #nosec in code. It can be a quick way to suppress security warnings and move forward with development, it should be employed with caution. Suppressing warnings with #nosec can hide potentially serious vulnerabilities. Only use #nosec when you're absolutely certain that the security issue is either a false positive or has been mitigated in another way.

Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203
Broad #nosec annotations should be avoided, as they can hide other vulnerabilities. The CI will block you from merging this PR until you remove #nosec annotations that do not target specific rules.

Pay extra attention to the way #nosec is being used in the files listed above.

@github-actions github-actions bot added the nosec label Oct 16, 2023
Copy link
Contributor

@ws4charlie ws4charlie left a comment

Choose a reason for hiding this comment

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

looks good to me

@lumtis lumtis mentioned this pull request Oct 16, 2023
@lumtis
Copy link
Member

lumtis commented Oct 16, 2023

@kingpinXD looks like there is a Lint issue blocking the merge

@kingpinXD kingpinXD merged commit 0a9d506 into develop Oct 16, 2023
11 checks passed
@kingpinXD kingpinXD deleted the tss-migration-cctx branch October 16, 2023 21:49
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.

Add new CCTX , of type admin cmd
4 participants