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/dkg contract interaction #3989

Merged
merged 8 commits into from
Oct 12, 2023
Merged

Feat/dkg contract interaction #3989

merged 8 commits into from
Oct 12, 2023

Conversation

soju-drinker
Copy link
Contributor

@soju-drinker soju-drinker commented Oct 5, 2023

Description

This PR adds the ability for the Signer to interact with Clarity contracts

Applicable issues

Checklist

  • Read only contract calls
  • Submit tx contract calls
  • Simulate contract call responses in tests

@soju-drinker soju-drinker changed the base branch from master to develop October 5, 2023 14:07
@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #3989 (c9b6528) into develop (f6668d8) will decrease coverage by 0.01%.
Report is 1 commits behind head on develop.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #3989      +/-   ##
===========================================
- Coverage     0.16%    0.16%   -0.01%     
===========================================
  Files          337      337              
  Lines       286814   287084     +270     
===========================================
  Hits           469      469              
- Misses      286345   286615     +270     
Files Coverage Δ
stacks-signer/src/config.rs 0.00% <0.00%> (ø)
stacks-signer/src/stacks_client.rs 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@soju-drinker soju-drinker marked this pull request as ready for review October 6, 2023 14:42
@soju-drinker soju-drinker force-pushed the feat/dkg-contract-interaction branch 2 times, most recently from c950efd to 4167979 Compare October 6, 2023 14:55
stacks-signer/Cargo.toml Outdated Show resolved Hide resolved
)
}

fn submit_tx(http_origin: &str, tx: &Vec<u8>) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this is ever going to be a concern, but if you're registering a BNS name, then you'd also need to supply an Attachment which contains the name's zonefile data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not entirely sure what this means XD Forgive me...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so there's this storage system in the Stacks node called Atlas It's a hold-over from the Blockstack / Stacks 1.0 days. Basically, a smart contract can emit specially-crafted Atlas events which contain the hash of a chunk of data. The Atlas system tracks these hash announcements, and then communicates with other Stacks nodes to see if they have a chunk of data which hashes to it. Eventually, all nodes get a copy of this "attached" data. We use it in BNS to tie a BNS name to a zone file, as well as other off-chain data like Nostr public keys and such.

I don't think the signer is ever going to use Atlas, so it's not a big deal if you're not going to support attachments. However, if this module becomes more generic and gets spun off into its own crate, it might become important later to add support.

Cargo.lock Show resolved Hide resolved
@soju-drinker
Copy link
Contributor Author

Pulled out the ureq suggestions into a separate issue - #3995

fn read_only_contract_call(
&self,
contract_addr: &StacksAddress,
contract_name: &str,
Copy link
Member

Choose a reason for hiding this comment

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

We have specific containers for contract names, function names, and function arguments, since they have to use limited character sets and since not all strings are valid. You should use ContractName and ClarityName here instead of a bare &str. Also, function_args should be &[clarity::vm::Value] in both cases.

Copy link
Collaborator

@jferrant jferrant Oct 10, 2023

Choose a reason for hiding this comment

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

Quick note, can you create a valid "StacksAddress" from the pox contract address? My initial attempts have failed. The reason I changed this to String was to accommodate this. However, perhaps I am just doing it wrong? Attempts to convert ""ST000000000000000000002AMW42H.pox-3" to a QualifiedContractID fail. Further attempts to split the string and then convert the first half to a StacksAddress also fails.

Copy link
Member

Choose a reason for hiding this comment

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

Sure -- QualifiedContractIdentifier::parse("ST000000000000000000002AMW42H.pox-3").unwrap() should work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I was not clear. This is what I already tried and it results in
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Runtime(ParseError("Invalid principal literal: base58ck checksum 0x51104e95 does not match expected 0x31287a31"), None)', stacks-signer/src/stacks_client.rs:315:83

This is the same result as trying to parse ST000000000000000000002AMW42H as a StacksAddress.

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

This PR is on the right track, but it needs a few changes:

  • It needs to use ContractName, ClarityName, and clarity::vm::Value instead of bare &strs
  • The StacksContractCallable trait may not need to exist

soju-drinker and others added 7 commits October 10, 2023 14:27
@jferrant jferrant force-pushed the feat/dkg-contract-interaction branch from e29466a to 0dd83c7 Compare October 10, 2023 21:29
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Copy link
Collaborator

@jferrant jferrant left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@xoloki xoloki left a comment

Choose a reason for hiding this comment

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

LGTM. More tests would be nice, especially if they weren't mocked, but this is fine for now.

@xoloki xoloki merged commit b6578ac into develop Oct 12, 2023
2 checks passed
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.

None yet

5 participants