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

[types] Deprecate Transaction::Program #923

Merged
merged 2 commits into from Sep 16, 2019

Conversation

@runtian-zhou
Copy link
Contributor

runtian-zhou commented Sep 12, 2019

Motivation

This diff is going to use our new model of Transaction::Script and Transaction::Module to do simple payment transactions in the libra codebase. Previously the normal transaction has one type of payload: Transaction::Program and one can publish a module AND execute some script within one single transaction. This feature however, is complicating the VM a lot and can really buy us few things as the modules are stateless. In our new model, we will have two types of transactions: Transaction::Module which one can use to publish a module and do nothing else, and Transaction::Script which one can execute code that mutates the resources on chain.

This diff will NOT be a breaking change as we still keep the functionality of processing Transaction::Program in the VM so that we will have some time to break the backward incompatibility there.

I removed the repl in this PR as well. Reason being that the local cli will have a superset of the functionality provided by the repl.

Have you read the Contributing Guidelines on pull requests?

(Write your answer here.)

Test Plan

(Share your test plan here. If you changed code, please provide us with clear instructions for verifying that your changes work.)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/libra/website, and link to your PR here.)

@review-assign-bot review-assign-bot bot requested a review from DavidLDill Sep 12, 2019
@runtian-zhou runtian-zhou force-pushed the runtian-zhou:deprecate_program branch from 38e51f0 to ee6c603 Sep 12, 2019
@bors-libra

This comment has been minimized.

Copy link
Collaborator

bors-libra commented Sep 12, 2019

☔️ The latest upstream changes (presumably 7f2ca75) made this pull request unmergeable. Please resolve the merge conflicts.

This diff is going to use our new model of Transaction::Script and Transaction::Module to do simple payment transactions in the libra codebase. This diff will NOT be a breaking change though as we still keep the functionality of processing Transaction::Program in the VM so that we will have some time to break the backward incompatability there.
@runtian-zhou runtian-zhou force-pushed the runtian-zhou:deprecate_program branch from ee6c603 to f20bdaf Sep 13, 2019
@runtian-zhou runtian-zhou requested a review from davidiw Sep 13, 2019
Copy link
Contributor

dariorussi left a comment

this looks great to me. As mentioned in a comment I wonder if we should also rename a bunch of variables from program to script though not critical.
Also we need to fix the linter so waiting for a working update to accept.
Review of the canonical serializer would be good @davidiw though it looks fine to me

@@ -89,18 +89,12 @@ impl<'a> Compiler<'a> {
}

/// Compiles the code and arguments into a `Program` -- the bytecode is serialized.
pub fn into_program(mut self, args: Vec<TransactionArgument>) -> Result<Program> {
pub fn into_program(mut self, args: Vec<TransactionArgument>) -> Result<Script> {

This comment has been minimized.

Copy link
@dariorussi

dariorussi Sep 13, 2019

Contributor

should we call this into_script? and fix the comment as well?

This comment has been minimized.

Copy link
@runtian-zhou

runtian-zhou Sep 13, 2019

Author Contributor

The reason why I didn't rename it to into_script is because we've already have input_script in the scope.

),
},
_ => panic!("Signed transaction payload arguments must have two arguments."),
},
_ => panic!("Signed transaction payload expected to be of struct Program"),

This comment has been minimized.

Copy link
@dariorussi

dariorussi Sep 13, 2019

Contributor

change Program to Script?

// Test helper for transaction creation
pub fn get_test_signed_transaction(
sender: AccountAddress,
sequence_number: u64,
private_key: Ed25519PrivateKey,
public_key: Ed25519PublicKey,
program: Option<Program>,
program: Option<Script>,

This comment has been minimized.

Copy link
@dariorussi

dariorussi Sep 13, 2019

Contributor

should we also change all those variables from program to script?

@cargo-dep-bot

This comment has been minimized.

Copy link

cargo-dep-bot bot commented Sep 13, 2019

This PR made the following dependency changes:

Removed Packages (Remaining versions in '()'):
	getopts 0.2.21
	repl 0.1.0

@phoenix-antigravity

This comment has been minimized.

Copy link
Contributor

phoenix-antigravity commented Sep 13, 2019

CLI part lgtm! I see that syntax has changed a bit. Should we update our online documentation? cc: @archjayas

Copy link
Contributor

dariorussi left a comment

LGTM, let's follow up on the online doc change if any

Copy link
Contributor

davidiw left a comment

lgtm for canonical

@davidiw

This comment has been minimized.

Copy link
Contributor

davidiw commented Sep 16, 2019

@runtian-zhou runtian-zhou merged commit 27615dc into libra:master Sep 16, 2019
1 check passed
1 check passed
commit-workflow Workflow: commit-workflow
Details
@runtian-zhou runtian-zhou deleted the runtian-zhou:deprecate_program branch Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.