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

Add contract update method that doesn't panic #2700

Closed
sisyphusSmiling opened this issue Aug 7, 2023 · 8 comments
Closed

Add contract update method that doesn't panic #2700

sisyphusSmiling opened this issue Aug 7, 2023 · 8 comments
Assignees

Comments

@sisyphusSmiling
Copy link
Contributor

Issue to be solved

Currently, there is no way to attempt to deploy or update a contract without a failure resulting in a panic as in Contracts.update__experimental(). This approach makes sense for the most part, but I found it can create difficulties when attempting to deploy/update multiple contracts as is the case in my exploration of delegated contract updates via onchain resources.

Suggested Solution

As discussed in this linked Discord thread, we might add some method(s) tryAdd()/tryUpdate() that adds/updates a contract, returning some structured data reflecting the deployment state. An example might be

pub enum ErrorType: UInt8 {
  pub case CONTRACT_NOT_FOUND
  pub case MULTIPLE_CONTRACTS_DECLARED
  pub case MISMATCHED_NAME
  pub case UNDEFINED
}

pub struct DeploymentError {
  pub let errorType: ErrorType
  pub let errorMessage: String
}

pub struct DeploymentResult {
  pub let success: Bool
  pub let errorMessage: DeploymentError?
}

pub fun tryUpdate(name: String, code: [UInt8]): DeploymentResult

This is submitted not to be prescriptive, but as an example of what the structured data value might look like. @bjartek might have some thoughts here as well.

@bjartek
Copy link
Contributor

bjartek commented Aug 8, 2023

This result object works for me.

@turbolent
Copy link
Member

turbolent commented Aug 8, 2023

Making the error message part of the result object is desirable, but is currently problematic, because this proposal (DeploymentError) will bring them on-chain / make them part of the execution result.

Currently, error messages are not part of the execution result (yet), because that in itself is a challenge, as errors will need to be fully deterministic in all their details.

There is a related effort to make it possible to include errors in the execution result in #2689, by standardizing a set of errors and error details that same versions of Cadence must produce.

@joshuahannan
Copy link
Member

Agreed that this would be pretty useful to have to automate upgrades. Could be especially useful in a DeFi scenario if a project has a bunch of swap token contracts to upgrade

@j1010001
Copy link
Member

@sisyphusSmiling is this blocking the contract upgrade for stable Cadence, or is it a "nice to have" ?

@SupunS
Copy link
Member

SupunS commented Aug 10, 2023

One alternative option is to have the update__experimental method return an optional DeployedContract?, so a nil result would indicate that something went wrong.

But then, of course, the developer would have to re-check the contract locally to see what is the actual issue.

@sisyphusSmiling
Copy link
Contributor Author

sisyphusSmiling commented Aug 10, 2023

@j1010001 not a blocker per se, but complexity increases significantly without it.

We'll either need to submit a transaction for every single update (meaning infra for proposal key queuing/rotation) or be very sure of update success when we call for an update (i.e. emulating updates prior to the spork against a mirror of mainnet).

The ideal case is we can iterate through batches of updates over the course of a handful of transactions. However, the potential for errors in failed updates throws a wrench in that planned iteration, meaning we'll need to submit updates individually.

@turbolent
Copy link
Member

Suggestion from @SupunS: success: Bool should probably be contract: DeployedContract?, so we still get access to the DeployedContract result, like update returns.

@turbolent
Copy link
Member

Until #2689 is ready, this API could already be implemented by leaving DeploymentResult.errorMessage out, and adding it once possible.

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

No branches or pull requests

6 participants