Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

Next Version of sBTC Clarity Scaffolding #354

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

setzeus
Copy link
Collaborator

@setzeus setzeus commented Nov 5, 2023

Scaffolding For Next Version of sBTC Clarity Contracts

Summary of Changes

This PR is tied to #353 & is an attempt to scaffold the next version of sBTC Clarity contracts based on the design researched & reviewed by the community. There is no code yet in any of the contracts.

Testing

No testing as none of the contracts included here have anything to test.

Risks

This appears relatively risk-free since the only possible this can effect Romeo is if a directory change broke items needed along some process.

How were these changes tested?

They weren't since there is no code inside any of these contracts yet.

Checklist:

  • [x ] My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

clarity/next/package.json Outdated Show resolved Hide resolved
clarity/next/tests/sbtc-db_test.ts Outdated Show resolved Hide resolved
@friedger
Copy link
Collaborator

friedger commented Nov 7, 2023

Why next folder? Will there be a main folder as well?

@setzeus
Copy link
Collaborator Author

setzeus commented Nov 14, 2023

@friedger negative, changing to sbtc-nakamoto. Made that name as a place holder.

moodmosaic and others added 2 commits November 15, 2023 17:03
# Conflicts:
#	clarity/romeo/asset-contract/tests/asset_GetBitcoinWalletPublicKeyCommand.ts
#	clarity/romeo/asset-contract/tests/asset_SetBitcoinWalletPublicKeyCommand.ts
#	clarity/romeo/asset-contract/tests/asset_SetBitcoinWalletPublicKeyCommand_NonOwner.ts
moodmosaic
moodmosaic previously approved these changes Nov 15, 2023
Copy link
Collaborator

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

LGTM (for the changes in romeo/asset-contract) stacks-network/sbtc#354 (comment)

Copy link
Collaborator

@friedger friedger left a comment

Choose a reason for hiding this comment

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

Why is romeo rust binary under clarity?
Top-level clarity folder does not make sense to me :-)

# epoch_2_4 = 105


# Send some stacking orders
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need stacking order here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Clarinet.toml is missing

Copy link
Collaborator

@friedger friedger left a comment

Choose a reason for hiding this comment

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

What is the acceptance criteria here?
Should we at least be able to run clarinet check and yarn test

@@ -0,0 +1,23 @@
{
"name": "next-tests",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better name here

@setzeus
Copy link
Collaborator Author

setzeus commented Nov 15, 2023

Why is romeo rust binary under clarity? Top-level clarity folder does not make sense to me :-)

The idea here is to create a single Clarity directory that holds different versions of sBTC - both the previous (Romeo) & new (Nakamoto) versions in this case.

Re: Rust binary was already in /src file in when copied over but you're right, we might as well remove /src & other files in that level since all Clarity items are in the "asset-contract" directory.

@moodmosaic
Copy link
Collaborator

moodmosaic commented Nov 15, 2023

What is the acceptance criteria here? Should we at least be able to run clarinet check and yarn test

@friedger I checked both when I rebased (and force-pushed) for romeo/asset-contract using Clarinet 1.8.0:

$ clarinet check
✔ 5 contracts checked

$ npm install && npm test

 ✓ tests/asset.test.ts (6) 26195ms
 ✓ tests/clarity-bitcoin-mini-deploy.test.ts (1)

And we still need to port the stuff in ext and scripts for the tests written in Clarity so they can run with the new SDK.

@moodmosaic moodmosaic dismissed their stale review November 15, 2023 16:54

Reviewed the changes in romeo/asset-contract and it appears that a wider review is warranted.

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2023

CLA assistant check
All committers have signed the CLA.

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

Successfully merging this pull request may close these issues.

4 participants