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 moonlight transactions #1930

Merged
merged 12 commits into from
Jul 22, 2024
Merged

Add moonlight transactions #1930

merged 12 commits into from
Jul 22, 2024

Conversation

ureeves
Copy link
Member

@ureeves ureeves commented Jul 4, 2024

execution-core: add Moonlight and rework withdrawals

With this commit we add MoonlightTransaction, and rename the existing
transaction to PhoenixTransaction, both of which are incorporated into
an enum named Transaction.

We also rework the withdrawals system, under the assumption that there
must be a strong assurance that the transacting party own the addresses
being withdraw to. This enables cross-withdrawals from Moonlight to
Phoenix (and vice-versa), as well as withdrawals to the same transaction
model.

The liberty is taken to change the stake data structures slighty, to
improve on clarity, as well as make use of the new withdrawal system.

transfer-contract: support for moonlight and expanded withdrawals

We add support for Moonlight transactions by adding a map between public
keys and their respective account data as defined by execution-core.
This means also adding a query function for this account data - named
account, taking as argument a public key.

Support for the new withdrawal system is also added, in a way that we
can reuse code between withdraw and mint, effectively minimizing the
amount of code written. One downside of this new system is that
contracts are no longer able to set data in the Sender of a Phoenix
note. Instead the tranfer contract is responsible for setting it, and
currently we set the contract ID withdrawn from, together with a
suffixed string.

We take the liberty to rework the transient data held during a call ray,
effectively minimizing data being passed back and forth - for instance
during a refund call. We also rename balance to contract_balance
contract-wide, to avoid confusion with Moonlight accounts.

stake-contract: rework to use new withdrawal system

We rework the contract to make use of the changed data structures. Most
notably, mint and withdraw now make use of the new withdrawals
system.

rusk-recovery: change to handle moonlight on genesis

To handle placing moonlight balances during state recovery, we add a new
moonlight_account item than a .toml configuration file can specify a
balance for given accounts at genesis. To ensure clarity we also rename
the balance item to phoenix_balance.

rusk: support Moonlight transactions

To support Moonlight transactions, we leverage the changes made to
execution-core to unify the handling of both transaction models using
the exact same code path as before.

Resolves: #1723, #1856
Closes: #1812

@ureeves ureeves force-pushed the moonlight branch 2 times, most recently from f80e5f4 to 90cc7d9 Compare July 10, 2024 23:52
@ureeves ureeves marked this pull request as ready for review July 11, 2024 00:02
consensus/src/user/provisioners.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@fed-franz fed-franz left a comment

Choose a reason for hiding this comment

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

I just reviewed the changes to consensus for now. I'll review the ones related to moonlight in another moment.

If I may, I'd suggest splitting the PR when it gets this big and there are "unrelated" changes. For instance, in this case, it would have helped having a PR with the changes to consensus, and then one based on the previous PR's branch with the moonlight changes.
This is just a suggestion to ease (and actually improving) the reviewing process in the future.

execution-core/src/lib.rs Outdated Show resolved Hide resolved
execution-core/src/stake.rs Outdated Show resolved Hide resolved
execution-core/src/stake.rs Outdated Show resolved Hide resolved
execution-core/src/stake.rs Outdated Show resolved Hide resolved
execution-core/src/stake.rs Outdated Show resolved Hide resolved
execution-core/src/stake.rs Outdated Show resolved Hide resolved
execution-core/src/transfer.rs Outdated Show resolved Hide resolved
node/src/chain/acceptor.rs Outdated Show resolved Hide resolved
node/src/chain/header_validation.rs Outdated Show resolved Hide resolved
node/src/chain/consensus.rs Outdated Show resolved Hide resolved
@ureeves
Copy link
Member Author

ureeves commented Jul 12, 2024

@herr-seppia, @fed-franz, @vlopes11 and I had a short meeting, and we all agreed the scope of the PR is far too large, and that the changes in the consensus that it introduces are wrong. To address this I'm setting the PR as being a draft while taking the changes targeting the consensus out.
Will remark as ready once done

@ureeves ureeves marked this pull request as draft July 12, 2024 13:19
@ureeves ureeves force-pushed the moonlight branch 4 times, most recently from 4540b35 to ccd46b2 Compare July 15, 2024 12:00
@ureeves
Copy link
Member Author

ureeves commented Jul 15, 2024

Alright. I've removed the stake changes, and addressed all comments that were already made. I'm reopening this for review once more.

@ureeves ureeves marked this pull request as ready for review July 15, 2024 12:08
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.

Generally really nice work!
Apart from some minor nits, I do want to raise the naming and creation of new structs.

  • There are two Withdraw structs (one in the stake and one in the transfer module of execution-core) which to me is highly confusing. If they serve different purposed, this should be reflected in their names as well, otherwise the stake contract can use the Withdraw from the transfer module.
  • Another point is the introduction of the two WithdrawPayload and the StakePayload structs. What is the reason why the structs exist? Previously the fields of the now ...Payload structs were part of the bigger struct. To me adding different payloads for different contexts (especially when there are two different Withdraw structs) is very confusing as I always have to look up which specific payload is meant in each context.

And: I totally love the introduction of transitory for the transfer-contract!

rusk-recovery/config/example_gov.toml Outdated Show resolved Hide resolved
rusk-recovery/src/state.rs Show resolved Hide resolved
rusk-recovery/src/state.rs Show resolved Hide resolved
rusk-recovery/src/state/snapshot.rs Show resolved Hide resolved
rusk-recovery/src/state/snapshot.rs Show resolved Hide resolved
execution-core/src/transfer/transaction.rs Outdated Show resolved Hide resolved
contracts/transfer/src/state.rs Outdated Show resolved Hide resolved
contracts/transfer/src/state.rs Outdated Show resolved Hide resolved
contracts/transfer/src/state.rs Show resolved Hide resolved
contracts/transfer/src/state.rs Show resolved Hide resolved
herr-seppia
herr-seppia previously approved these changes Jul 19, 2024
fed-franz
fed-franz previously approved these changes Jul 19, 2024
Eduardo Leegwater Simões added 11 commits July 20, 2024 22:32
With this commit we add `MoonlightTransaction`, and rename the existing
transaction to `PhoenixTransaction`, both of which are incorporated into
an enum named `Transaction`.

We also rework the withdrawals system, under the assumption that there
must be a strong assurance that the transacting party own the addresses
being withdraw to. This enables cross-withdrawals from Moonlight to
Phoenix (and vice-versa), as well as withdrawals to the same transaction
model.

The liberty is taken to change the stake data structures slighty, to
improve on clarity, as well as make use of the new withdrawal system.
We add support for Moonlight transactions by adding a map between public
keys and their respective account data as defined by `execution-core`.
This means also adding a query function for this account data - named
`account`, taking as argument a public key.

Support for the new withdrawal system is also added, in a way that we
can reuse code between `withdraw` and `mint`, effectively minimizing the
amount of code written. One downside of this new system is that
contracts are no longer able to set data in the `Sender` of a Phoenix
note. Instead the tranfer contract is responsible for setting it, and
currently we set the contract ID withdrawn from, together with a
suffixed string.

We take the liberty to rework the transient data held during a call ray,
effectively minimizing data being passed back and forth - for instance
during a `refund` call. We also rename `balance` to `contract_balance`
contract-wide, to avoid confusion with Moonlight accounts.
We rework the contract to make use of the changed data structures. Most
notably, `mint` and `withdraw` now make use of the new withdrawals
system.
To handle placing moonlight balances during state recovery, we add a new
`moonlight_account` item than a `.toml` configuration file can specify a
balance for given accounts at genesis. To ensure clarity we also rename
the `balance` item to `phoenix_balance`.
@ureeves ureeves dismissed stale reviews from fed-franz, herr-seppia, and moCello via 20a4a55 July 20, 2024 20:32
@moCello moCello self-requested a review July 22, 2024 08:04
moCello
moCello previously approved these changes Jul 22, 2024
To support Moonlight transactions, we leverage the changes made to
`execution-core` to unify the handling of both transaction models using
the exact same code path as before.
Copy link
Contributor

@miloszm miloszm left a comment

Choose a reason for hiding this comment

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

Looks Very Good!

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.

Implement genesis contract Account for Moonlight Shard
9 participants