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: BTC contract swap encoding #5311

Merged
merged 10 commits into from
Oct 11, 2024
Merged

Conversation

msgmaxim
Copy link
Contributor

@msgmaxim msgmaxim commented Oct 3, 2024

Pull Request

Closes: PRO-1673

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have written sufficient tests.

Summary

  • Defines UtxoEncodedData which effectively determines how swap parameters are layed out/encoded. For encoding/decoding we use Scale to keep things smiple. It still results in a reasonably compact encoding: the check_utxo_encoding test demonstrates which bytes are used for what (and that we don't exceed the limit of 80 bytes). I chose not to use "insta" in this case to make things a bit more explicit.
  • SharedCfParameters defines a subset of UtxoEncodedData's fields which we expect to use in other smart contracts (this is what you intended @albert-llimos?)
  • As discussed on discord, min price is encoded as min output amount to save space. The encoding of other fields is more or less straightforward.

@msgmaxim msgmaxim requested a review from kylezs as a code owner October 3, 2024 06:31
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 86.48649% with 35 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (ab7104b) to head (cdde79e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
engine/src/witness/btc/smart_contract.rs 92% 12 Missing and 3 partials ⚠️
...te-chain/chains/src/btc/smart_contract_encoding.rs 78% 11 Missing and 3 partials ⚠️
state-chain/chains/src/btc.rs 60% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #5311    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        488     490     +2     
  Lines      84898   85016   +118     
  Branches   84898   85016   +118     
======================================
+ Hits       60375   60405    +30     
- Misses     21822   21897    +75     
- Partials    2701    2714    +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@albert-llimos
Copy link
Contributor

albert-llimos commented Oct 3, 2024

SharedCfParameters defines a subset of UtxoEncodedData's fields which we expect to use in other smart contracts (this is what you intended @albert-llimos?)

Yes exactly, that looks good to me.

@albert-llimos
Copy link
Contributor

The only thing left to "match" the contract swaps is the CCM parameters (message and gasAmount). I understand that we have this 80bytes limitation so it definitely doesn't fit there. I guess there might be ways around it (e.g. add another utxo for that, although still limited in length) but I assume we don't care about supporting CCM swaps, especially right now. I wouldn't care but just double checking that we're all aligned. @martin-chainflip @msgmaxim

@msgmaxim msgmaxim changed the title feat: BTC constract swap encoding feat: BTC contract swap encoding Oct 8, 2024
engine/src/witness/btc/smart_contract.rs Show resolved Hide resolved
engine/src/witness/btc/smart_contract.rs Show resolved Hide resolved
state-chain/chains/src/btc.rs Outdated Show resolved Hide resolved
state-chain/chains/src/btc.rs Show resolved Hide resolved
state-chain/chains/src/btc/smart_contract_encoding.rs Outdated Show resolved Hide resolved
state-chain/chains/src/btc/smart_contract_encoding.rs Outdated Show resolved Hide resolved
state-chain/chains/src/btc/smart_contract_encoding.rs Outdated Show resolved Hide resolved
state-chain/chains/src/btc/smart_contract_encoding.rs Outdated Show resolved Hide resolved
engine/src/witness/btc/smart_contract.rs Show resolved Hide resolved
@msgmaxim msgmaxim force-pushed the feat/btc-smart-contract-encoding branch from 3e1967e to 7cc4533 Compare October 10, 2024 02:56
Comment on lines +989 to +991
// We only store (spendable) BitcoinScripts that we authored ourselves.
// For these, the max encoded length is MAX_BITCOIN_SCRIPT_LENGTH.
// For anything else, we don't need MaxEncodedLen.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed that PushBytes allows MAX_PUSHABLE_BYTES, but we were storing it in a bounded vec of size MAX_BITCOIN_SCRIPT_LENGTH.

The only reason we defined this was to derive MaxEncodedLen, so I moved the definition of this here.

Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

LGTM - I added one small change (see comment). Will let you double-check my commit and then push the button if you're happy.

@msgmaxim msgmaxim enabled auto-merge October 11, 2024 00:06
@msgmaxim msgmaxim added this pull request to the merge queue Oct 11, 2024
Merged via the queue into main with commit 5a8a382 Oct 11, 2024
49 checks passed
@msgmaxim msgmaxim deleted the feat/btc-smart-contract-encoding branch October 11, 2024 01:17
syan095 added a commit that referenced this pull request Oct 14, 2024
…lana-ccm

* origin/main:
  ci: upgrade action version to supress deprecation warnings ⚙️ (#5330)
  feat: handle rotation tx construction failures (#5307)
  test(bouncer): add test for new utility (#5324)
  fix: keyholder check should use `HistoricalActiveEpochs` (#5325)
  feat: BTC contract swap encoding (#5311)
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.

4 participants