Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

Implement applyMsg method #31

Merged
merged 14 commits into from
Apr 26, 2023
Merged

Implement applyMsg method #31

merged 14 commits into from
Apr 26, 2023

Conversation

arrusev
Copy link
Contributor

@arrusev arrusev commented Mar 31, 2023

  • add applyMsg method that triggers the execution of a cross-subnet message
  • add system actor validation
  • optimize the postbox storage

This was referenced Mar 31, 2023
@arrusev
Copy link
Contributor Author

arrusev commented Mar 31, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@mikirov mikirov force-pushed the feature/method-apply-msg branch 2 times, most recently from d94ac57 to 67b5a3b Compare April 9, 2023 20:23
@mikirov mikirov mentioned this pull request Apr 9, 2023
@arrusev arrusev force-pushed the feature/method-apply-msg branch 6 times, most recently from 9aff9d6 to 2c22b6f Compare April 12, 2023 07:31
@2dpetkov 2dpetkov mentioned this pull request Apr 12, 2023
@mikirov mikirov self-assigned this Apr 20, 2023
mikirov
mikirov previously approved these changes Apr 20, 2023
README.md Outdated Show resolved Hide resolved
foundry.toml Outdated Show resolved Hide resolved
src/Gateway.sol Outdated Show resolved Hide resolved
src/Gateway.sol Outdated Show resolved Hide resolved
src/SubnetActor.sol Outdated Show resolved Hide resolved
src/interfaces/IGateway.sol Outdated Show resolved Hide resolved
@arrusev arrusev changed the base branch from main to feature/cross-msg April 21, 2023 07:30
@arrusev arrusev changed the base branch from feature/cross-msg to main April 21, 2023 07:31
@arrusev arrusev dismissed mikirov’s stale review April 21, 2023 07:31

The base branch was changed.

Signed-off-by: Anton Rusev <anton.rusev@limechain.tech>
@2dpetkov 2dpetkov self-requested a review April 21, 2023 09:48
Copy link
Contributor

@2dpetkov 2dpetkov left a comment

Choose a reason for hiding this comment

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

Left some comments and nitpicks. And a discussion point.

src/Gateway.sol Outdated Show resolved Hide resolved
src/Gateway.sol Outdated Show resolved Hide resolved
src/Gateway.sol Outdated Show resolved Hide resolved
src/constants/Constants.sol Outdated Show resolved Hide resolved
test/Gateway.t.sol Outdated Show resolved Hide resolved
src/lib/CrossMsgHelper.sol Show resolved Hide resolved
Signed-off-by: Anton Rusev <anton.rusev@limechain.tech>
Signed-off-by: Anton Rusev <anton.rusev@limechain.tech>
@2dpetkov 2dpetkov self-requested a review April 24, 2023 11:38
2dpetkov
2dpetkov previously approved these changes Apr 24, 2023
Copy link
Contributor

@2dpetkov 2dpetkov left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

mikirov
mikirov previously approved these changes Apr 24, 2023
@2dpetkov 2dpetkov requested a review from adlrocha April 25, 2023 07:04
adlrocha
adlrocha previously approved these changes Apr 25, 2023
Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

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

Great job! LGTM. Really looking forward to the refactor so we can simplify the code a bit:)

int64 constant DEFAULT_CHECKPOINT_PERIOD = 10;

/// @notice minimum amount of wei to register a subnet
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should check this once we try to deploy the smart contract in Filecoin, but I don't know how are Filecoin units when sent in a payable transaction considered by the smart contract. Maybe we should use the Filecoin Solidity type for this.

Copy link
Contributor

@mikirov mikirov Apr 25, 2023

Choose a reason for hiding this comment

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

The fevmate library has a WFIL implementation that is an ERC20 with 18 decimals, same as ether. This brings me to the thought that if wrapped FIL has 18 decimals the native one may have 18 as well?


IPCMsgType applyType = crossMsg.message.applyType(networkName);

if (crossMsg.message.to.subnetId.equals(networkName)) {
Copy link
Contributor

@adlrocha adlrocha Apr 25, 2023

Choose a reason for hiding this comment

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

Suggested change
if (crossMsg.message.to.subnetId.equals(networkName)) {
// If the cross-message destination is the current network.
if (crossMsg.message.to.subnetId.equals(networkName)) {


return crossMsg.execute();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// when the destination is not the current network we add it to the postbox for further propagation

function _bottomUpStateTransition(
StorableMsg calldata storableMsg
) internal {
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot how horrible this piece of code was 🤦 This will be significantly simplified after the refactor. The behavior is the same as with top-down messages where there is a unique nonce for each cross-net message, and for each subnet (instead of having to aggregate them as we do now)

Signed-off-by: Anton Rusev <anton.rusev@limechain.tech>
@arrusev arrusev dismissed stale reviews from adlrocha, mikirov, and 2dpetkov via 115e296 April 25, 2023 09:52
Copy link
Contributor

@mikirov mikirov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@2dpetkov 2dpetkov left a comment

Choose a reason for hiding this comment

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

beautiful :)

@arrusev arrusev merged commit 08adebe into main Apr 26, 2023
@dnkolegov dnkolegov deleted the feature/method-apply-msg branch June 27, 2023 11:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants