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: add new static method to contract Client to deploy and construct contracts #1086

Merged
merged 21 commits into from
Nov 13, 2024

Conversation

willemneal
Copy link
Member

This will allow using a wasm hash and constructor args to create an assembled transaction that will deploy and initialize a contract. Generated types in the CLI are to follow.

@willemneal willemneal changed the title feat: add new static method to deploy and construct contracts feat: add new static method to contract Client to deploy and construct contracts Oct 22, 2024
src/contract/client.ts Outdated Show resolved Hide resolved
src/contract/client.ts Outdated Show resolved Hide resolved
test/unit/spec/contract_spec.ts Outdated Show resolved Hide resolved
@chadoh
Copy link
Contributor

chadoh commented Oct 29, 2024

A Question on API Design

Which do you prefer? Also feel free to suggest other ideas!

Note: all examples below are using a simple increment-style contract that allows the counter's initial value to be set upon deploy.

Option 1 (Current Behavior)

Right now, we have it working like this:

// this returns an AssembledTransaction which, when sent, returns a `Client`
const deployTx = await contract.Client.deploy(
  { counter: INIT_VALUE },
  {
    networkPassphrase,
    rpcUrl,
    wasmHash,
    publicKey,
    signTransaction,
  },
);
const { result: client } = await deployTx.signAndSend();
const { result } = await client.counter();
expect(result, INIT_VALUE);

This makes maximum use of existing AssembledTransaction patterns.

Option 2

That const { result: client } = line is a little awkward, innit? Maybe we can wrap AssembledTransaction with a PendingClientAssembledTransaction that allows us to simplify it?

 const deployTx = await contract.Client.deploy(
   { counter: INIT_VALUE },
   {
     networkPassphrase,
     rpcUrl,
     wasmHash,
     publicKey,
     signTransaction,
   },
 );
-const { result: client } = await deployTx.signAndSend();
+const client = await deployTx.signAndSend();
 const { result } = await client.counter();
 expect(result, INIT_VALUE);

Option 3

But then, is it worth requiring people to call signAndSend? Should it be done automatically as part of contract.Client.deploy?

-const deployTx = await contract.Client.deploy(
+const client = await contract.Client.deploy(
   { counter: INIT_VALUE },
   {
     networkPassphrase,
     rpcUrl,
     wasmHash,
     publicKey,
     signTransaction,
   },
 );
-const { result: client } = await deployTx.signAndSend();
 const { result } = await client.counter();
 expect(result, INIT_VALUE);

@Shaptic
Copy link
Contributor

Shaptic commented Oct 29, 2024

@chadoh I kinda like (3) the most for ergonomics - what're the cons? The implicit sign-ingness?

@chadoh
Copy link
Contributor

chadoh commented Oct 29, 2024

@Shaptic yes, the implicit signingness.

@kalepail do you have opinions on the three options above?

@Shaptic
Copy link
Contributor

Shaptic commented Oct 29, 2024

They'd still have to approve it via Freighter et al. though, yeah? It's the most implicit for keypairs I guess but I think it's fine 🤷

@kalepail
Copy link
Contributor

I don't love the idea of being overly smart about it but as long as I have mechanics for overriding requiring the use of G-address signers on Freighter-like wallets I don't care that much.

I'm definitely in the camp where we'll start to see more and more use of smart wallets as signers and less and less external G-addresses via extensions, etc. So I will take the line we should plan for that and not assume we really know how folks will want to get these transactions signed or submitted and being too smart will lead to more of the issues we have now where the JS-SDK has to be wrestled against in many cases because it assumes too much.

@kalepail
Copy link
Contributor

Fwiw I don't see the use case being too common that folks will want to deploy and then invoke a contract back to back. There will be more time gaps between those actions. For that reason I think Option 1 is sufficient and the most sane / consistent with other behaviors. If there was something to change it would yeah be renaming result to client or something more inline with the idea this isn't a result from simulation or tx submission but a Client object.

@ElliotFriend
Copy link
Contributor

First, @chadoh, I love that your example contract is an increment contract that doesn't increment! 🤣

For the reasons @kalepail already pointed out, I'd lean toward option 1 or 2, where the signature/sending is still up to the builder's choices. Which is better? 🤷🏻‍♂️

I would probably disagree a bit with the usefulness of the approach, though. In the stuff I've built (which is mostly silly, tbh), it's been really useful to deploy/init immediately in a single, atomic transaction. The process of using the deployer example to pass in the init_args is far from painless, and took quite a bit of figuring out. The biggest use-case (for my stuff) has been to make a smoother UX. If someone's "creating a new game" (by deploying/initializing a contract), it's far easier/simpler to just click a button once, sign one transaction, and be done. The alternative would be "first you have to deploy your game. ok, now you've done that, let's initialize your game." Not the process I'd want as a user in that kind of flow. Of course, I can't speak for others on if they have/would use this kind of thing. And, how often will people have users deploying/initializing contracts for whatever reason 🤷🏻‍♂️

Then again, if you already know the Wasm hash (and you would have to if you're attempting a deploy/init thing), it's not that big of a hassle to re-deploy if someone "snatches" your uninitialized contract out from under you. That is assuming the only loss in the uninitialized contract is the fee you spent to deploy it. If something is hard-coded in the uninitialized contract that could lead to actual loss for you, (why would you write the contract that way?) then it is obviously a bigger deal.

@willemneal
Copy link
Member Author

The deployer contract was a stop gap measure imo. It adds a layer of abstraction for new users (I have to deploy a contract to deploy my contract?). Now we have a way to do both with the types!

Regardless we need to offer this to match the functionality in the CLI. I also think it will be great in the Lab when paired with contract invocation forms. So devs can deploy contracts with documentation and types!

@chadoh
Copy link
Contributor

chadoh commented Nov 1, 2024

@ElliotFriend thanks for the review! I'm not sure I understand your concern, though. It sounds like you are saying you disagree with this change (which aspect of it?), but then go on to sing its virtues. "We need easier deploy-and-init!" Well, yes, that's what this is!

@willemneal willemneal marked this pull request as ready for review November 1, 2024 17:49
@chadoh chadoh force-pushed the feat/deploy_constructor branch 2 times, most recently from b493046 to dd0e63a Compare November 1, 2024 19:14
@chadoh
Copy link
Contributor

chadoh commented Nov 8, 2024

Sticking with Option 1 for now, since it's the simplest to implement/maintain and it seems like maybe it won't matter to many people.

Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

Very nice 👏

explicit constructorArgs contract went away; it's now part of `increment`
@Shaptic Shaptic merged commit 40d1497 into stellar:master Nov 13, 2024
11 checks passed
@chadoh chadoh deleted the feat/deploy_constructor branch November 13, 2024 03:45
BlaineHeffron pushed a commit to AhaLabs/js-stellar-sdk that referenced this pull request Nov 13, 2024
…ntracts (stellar#1086)

This will allow using a wasm hash and constructor args to create
an assembled transaction that will deploy and initialize a contract.
Generated types in the CLI are to follow.

Co-authored-by: Elliot Voris <elliot@voris.me>
Co-authored-by: Chad Ostrowski <221614+chadoh@users.noreply.github.com>
Co-authored-by: Elliot Voris <elliot@voris.me>
@ElliotFriend
Copy link
Contributor

@chadoh Just realized I never responded to your comment!! So sorry! 🤦🏻‍♂️

What I meant was that I disagree with Tyler on his stance that Fwiw I don't see the use case being too common that folks will want to deploy and then invoke a contract back to back. I'm 100% on board with the change and stoked to have it in the SDK!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants