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: support versioned-smart-contract tx types #1341

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

zone117x
Copy link
Member

@zone117x zone117x commented Aug 18, 2022

This PR was published as an alpha to npm with the version 4.3.6-pr.e86b982.0 — e.g. npm install @stacks/common@4.3.6-pr.e86b982.0

Closes #1339

I need this change for some API testing & development for Stacks 2.1.

This adds an optional arg clarityVersion to the smart contract builder functions. If specified, a versioned-smart-contract type will be created. If not specified, it will default to the regular smart-contract.

Example:

const transaction = await makeContractDeploy({
  contractName,
  codeBody,
  senderKey,
  clarityVersion: ClarityVersion.Clarity2,
});

I have no strong feelings on how these function signatures look. An alternative approach could be using entirely new builder functions for this tx type, rather than an optional arg to the existing functions.

I verified that the serialized payload generated in this PR is valid with the Rust deserialization code at https://github.com/hirosystems/stacks-encoding-native-js/pull/5/files

@janniks feel free to take over this PR and make any changes

@vercel
Copy link

vercel bot commented Aug 18, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
stacksjs ✅ Ready (Inspect) Visit Preview Aug 29, 2022 at 10:32AM (UTC)

test('Versioned smart contract payload serialization and deserialization', () => {
const contractName = 'contract_name';
const codeBody =
'(define-map store ((key (buff 32))) ((value (buff 32))))' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell this is the came contract as fs.readFileSync('./tests/contracts/kv-store.clar'), we could reuse that file for this test as well...

Copy link
Collaborator

@janniks janniks left a comment

Choose a reason for hiding this comment

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

Clean, and fast 🏎

@janniks
Copy link
Collaborator

janniks commented Aug 18, 2022

Maybe we could add "test vector" style tests with hardcoded serialized txs (which match the rust impl). Thoughts? 💭

This PR will be deployed to npm via github actions, is that enough for testing and development? (I'm thinking about if it makes sense to hold off releasing these additions to be closer to 2.1 launch; maybe bundle them together) 🤷‍♂

@zone117x
Copy link
Member Author

@janniks I'm already using this code for local API testing and development. Deployment isn't a blocker for me. But yeah I think if we do deploy this, it would make more sense if it was tagged as alpha on npm. I don't think the Stacks 2.1 changes around all of this are finalized (the serialization format could change, but unlikely).

@janniks
Copy link
Collaborator

janniks commented Aug 24, 2022

📦 Released under alpha released as @4.4.0-stacks2.1-alpha.0

@zone117x zone117x changed the base branch from master to stacks-2.1 August 29, 2022 09:45
@zone117x
Copy link
Member Author

zone117x commented Aug 29, 2022

@janniks I just created a new branch stacks-2.1 (from master), and switched this PR to merge into that. That way, we can merge this one, then I can submit additional PRs against the stacks-2.1 branch until we're ready for release. Are you okay with that?

@zone117x zone117x force-pushed the feat/versioned-smart-contracts branch from 709d523 to e86b982 Compare August 29, 2022 10:31
@codecov-commenter
Copy link

Codecov Report

Merging #1341 (709d523) into stacks-2.1 (c5536b7) will increase coverage by 0.04%.
The diff coverage is 82.75%.

❗ Current head 709d523 differs from pull request most recent head e86b982. Consider uploading reports for the commit e86b982 to get more accurate results

@@              Coverage Diff               @@
##           stacks-2.1    #1341      +/-   ##
==============================================
+ Coverage       65.06%   65.10%   +0.04%     
==============================================
  Files             126      126              
  Lines            8833     8856      +23     
  Branches         1910     1919       +9     
==============================================
+ Hits             5747     5766      +19     
- Misses           2833     2837       +4     
  Partials          253      253              
Impacted Files Coverage Δ
packages/transactions/src/index.ts 100.00% <ø> (ø)
packages/transactions/src/transaction.ts 79.38% <0.00%> (-0.62%) ⬇️
packages/transactions/src/builders.ts 73.01% <40.00%> (-0.32%) ⬇️
packages/transactions/src/payload.ts 92.59% <94.44%> (+0.03%) ⬆️
packages/transactions/src/constants.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

[Stacks 2.1] Support versioned-smart-contract transactions
3 participants