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 deployment transaction #164

Closed
wants to merge 3 commits into from
Closed

Conversation

ureeves
Copy link
Member

@ureeves ureeves commented Apr 30, 2024

The deployment transaction contains the metadata necessary to deploy a contract, together with its bytecode and possible constructor arguments.

Resolves: #163

The deployment transaction contains the metadata necessary to deploy a
contract, together with its bytecode and possible constructor arguments.

Resolves: #163
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

Since we now have the transaction-types in the rusk contracts, wouldn't it make sense to add the Transaction there too? Ideally I would like to get rid of the mod transaction in phoenix-core all together.

Edit: I think that is part of a bigger discussion though and not strictly related to this PR.

@moCello moCello self-requested a review May 1, 2024 08:43
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

This LGTM
As far as I understand a Transaction can now be 3 different things:

  • a normal transaction (exec = None)
  • a call to a contract function (exec = Some(call))
  • a deployment of a new contract (exec = Some(deploy))

With the changes that are coming up in phoenix, we might need to rethink much of the overall design of the contract but that's for later ;)

src/transaction.rs Show resolved Hide resolved
bytecode,
constructor_args,
} => {
bytes.push(2);
Copy link

Choose a reason for hiding this comment

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

Since the encodings 0,1,2 are also used below for deserialisation, could we maybe introduce some implicit constants for them? Also, case 0 would be easier to understand if it had a name.

@ureeves
Copy link
Member Author

ureeves commented May 17, 2024

Not relevant anymore

@ureeves ureeves closed this May 17, 2024
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.

Add contract deployment transaction
3 participants