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

Upgrade to ink-4.0 #1063

Merged
merged 4 commits into from
Feb 9, 2023
Merged

Upgrade to ink-4.0 #1063

merged 4 commits into from
Feb 9, 2023

Conversation

kvinwang
Copy link
Collaborator

@kvinwang kvinwang commented Dec 7, 2022

This PR updates the pink SDK and drivers to use ink-4.0-rc.

Deprecating ink-3.0

The pallet-contract itself still supports running ink-3.0 contracts. However, due to a breaking change in chain extension ABI, we have changed one chain extension API, cache_set, which would break the previous compiled contracts.

The following conditions are required in order to run ink-3.0 contracts on pruntime after nightly-2023-02-10:

  • Upgrade pink-extension to 0.2.1 if the contract uses the API cache_set.
  • Set up the cluster with the system contract and drivers compiled with ink-3.0.

Changes required to JS front-end

Because ink-4.0 changed the message output from Result<T> to Result<Result<T, LangError>>, all front-end codes querying to contracts will have to update the result handling like this
cc @Leechael

TODO (external resources updates)

  • Update setup scripts (PR)
  • Publish crate pink-extension 0.4
  • Update phat-contract-examples
  • Update pink-web3
  • Update pink-quickjs

Base automatically changed from allow-indeterminism to master December 9, 2022 01:50
@kvinwang kvinwang changed the title pink: Towards ink-4.0-beta Upgrade to ink-4.0 Feb 6, 2023
@h4x3rotab
Copy link
Contributor

Because ink-4.0 changed the message output from Result to Result<Result<T, LangError>>, all front-end codes querying to contracts will have to update the result handling like this

This affects all the js-side code, including the frontend projects, and almost all the E2E tests involving contract returning results. A few listed:

  • Phat Offchain Rollup and Sub0 Workshop: E2E tests
  • Toolchains like Devphase and Swanky Phala Plugin
  • Internal Phat projects
  • Community projects: PrivaDex and others

@kvinwang
Copy link
Collaborator Author

kvinwang commented Feb 6, 2023

This affects all the js-side code, including the frontend projects, and almost all the E2E tests involving contract returning results. A few listed:

Could we wrap the query methods in js-sdk so that it hides the changes?

@Leechael
Copy link
Contributor

Leechael commented Feb 6, 2023

This affects all the js-side code, including the frontend projects, and almost all the E2E tests involving contract returning results. A few listed:

Could we wrap the query methods in js-sdk so that it hides the changes?

This affects all the js-side code, including the frontend projects, and almost all the E2E tests involving contract returning results. A few listed:

Could we wrap the query methods in js-sdk so that it hides the changes?

It's possible, but as it's a breaking change, can we detect that by version detection? (prevent connecting to a node not yet up-to-date).

@kvinwang
Copy link
Collaborator Author

kvinwang commented Feb 6, 2023

It's possible, but as it's a breaking change, can we detect that by version detection? (prevent connecting to a node not yet up-to-date).

We can bump the pruntime version. However, it's not related to the thing that I said wrap the query methods for.

wrap the query methods is to make it:

  • less painful (and fewer bugs) for projects which use js-sdk to migrate to talk to ink-4.0.
  • and maybe more flexibly adapt to future changes.

@kvinwang kvinwang marked this pull request as ready for review February 7, 2023 03:22
Copy link
Contributor

@h4x3rotab h4x3rotab left a comment

Choose a reason for hiding this comment

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

LGTM

"payable": false,
"selector": "0xed4b9d1b"
}
"spec": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Incompatible change for toolchains like Devphase and Phat UI, I guess. @Leechael

@@ -59,66 +124,66 @@ pub trait PinkExt {
/// Values stored in cache can only be read in query functions.
///
/// Alwasy returns `Ok(())` if it is called from a command context.
#[ink(extension = 6, handle_status = false, returns_result = false)]
#[ink(extension = 6, handle_status = true)]
Copy link
Contributor

Choose a reason for hiding this comment

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

What does handle_status mean?

Copy link
Collaborator Author

@kvinwang kvinwang Feb 9, 2023

Choose a reason for hiding this comment

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

When handle_status = true, ink framework would check the status code(which is just the return value of the ffi fn) and, if non-zero, convert it to the Result::Err.
When handle_status = false, it would decode the T of -> Result<T, E> directly without checking the status code, no way to pass the value of E.

In ink-3.0, we can use handle_status = false, returns_result = false to bypass the smart decoding mechanism and just let it decode the entire Result<T, E> from the output buffer. But the returns_result was removed in ink-4.0 and now auto detects it by the type system, so we have no other choices.

@kvinwang
Copy link
Collaborator Author

kvinwang commented Feb 9, 2023

This affects all the js-side code, including the frontend projects, and almost all the E2E tests involving contract returning results. A few listed:

Could we wrap the query methods in js-sdk so that it hides the changes?

@h4x3rotab How do you think about this?

@kvinwang kvinwang merged commit fca347e into master Feb 9, 2023
@kvinwang kvinwang deleted the ink-4.0 branch February 9, 2023 11:26
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.

3 participants