-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: support l2 plus #1157
base: main
Are you sure you want to change the base?
feat: support l2 plus #1157
Conversation
7696425
to
463ee74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
@@ -27,6 +27,7 @@ contract XnetMessagingFacet is GatewayActorModifiers { | |||
/// @dev It requires the caller to be the system actor. | |||
/// @param crossMsgs The array of cross-network messages to be applied. | |||
function applyCrossMessages(IpcEnvelope[] calldata crossMsgs) external systemActorOnly { | |||
LibGateway.applyMessages(s.networkName.getParentSubnet(), crossMsgs); | |||
LibGateway.applyTopDownMessages(s.networkName.getParentSubnet(), crossMsgs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applyCrossMessages
only handles topdown messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is only used to apply top-down messages locally after they have been finalized in Fendermint.
@@ -199,4 +203,8 @@ library CrossMsgHelper { | |||
|
|||
return true; | |||
} | |||
|
|||
function validateCrossMessage(IpcEnvelope memory crossMsg) internal view returns (CrossMessageValidationOutcome) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caller why not just invoke LibGateway
directly instead, it's internal anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raul has suggested a nice syntax sugar update. So I have added it after his comment.
function down(SubnetID calldata subnet1, SubnetID calldata subnet2) public pure returns (SubnetID memory) { | ||
if (subnet1.root != subnet2.root) { | ||
revert DifferentRootNetwork(); | ||
return SubnetID({root: 0, route: new address[](0)}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason why it's not reverted? Feels like it's an error that should be handled instead of introducing root = 0
as invalid subnet. Probably in some use cases root: 0, route: []
is a valid use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like this needs multi-return? SubnetID + bool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that if I revert, I lose control elsewhere. The method is also used invalidateCrossMessage
, where reverting is not desirable. Additionally, commonParent
method here returns the empty subnet ID instead of reverting. But I understand your point—if this were deployed on Ethereum, the root could be zero. Would returning a boolean (indicating whether it was found or not) help?
revert NotRegisteredSubnet(); | ||
} | ||
|
||
function commitTopDownMsg(Subnet storage subnet, IpcEnvelope memory crossMessage) internal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like we just need the subnet id as the param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. This also feels unsafe since this function could accidentally modify the subnet state, since it's passed with the storage
modifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can pass the subnetID down, but then I would need to look up the subnet again with getSubnet(subnetId), since this function actually updates the subnet state (and has been).
@@ -199,4 +203,8 @@ library CrossMsgHelper { | |||
|
|||
return true; | |||
} | |||
|
|||
function validateCrossMessage(IpcEnvelope memory crossMsg) internal view returns (CrossMessageValidationOutcome) { | |||
return LibGateway.validateCrossMessage(crossMsg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still kind of feel CrossMessageValidationOutcome
should not be exposed, unless different callers might handle the outcomes differently? Now every call of validateCrossMessage
must handle the outcome, dont feel it's necessary though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless different callers might handle the outcomes differently?
- that's exactly the case :)
}); | ||
|
||
address[] memory nativeSubnetPath; | ||
nativeSubnet = createNativeSubnet(nativeSubnetPath, rootNetwork.gatewayAddr, rootNetwork.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe call this nativeL2Subnet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to pass an empty address[]
as the first argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raulk Solved by using the parent route instead.
IpcEnvelope memory crossMessage = TestUtils.newXnetCallMsg( | ||
IPCAddress({subnetId: params.subnetL3.id, rawAddress: FvmAddressHelper.from(params.callerAddr)}), | ||
IPCAddress({subnetId: params.root.id, rawAddress: FvmAddressHelper.from(params.recipientAddr)}), | ||
params.amount, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the param amount could exceed the fundSubnet
amount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
// submit checkoint so the result message can be propagated to L2 | ||
submitBottomUpCheckpoint( | ||
callCreateBottomUpCheckpointFromChildSubnet(params.subnetL3.id, params.subnetL3.gateway), | ||
params.subnetL3.subnetActor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be subnetL2.subnetActor
? The checkpoint should be propagated to L2 right? Not to L3 subnet actor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params.subnetL3.subnetActor corresponds to the L2 subnet actor. I agree it can be confusing, but I'm building around the already existing TestSubnetDefinition
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if you find it super confusing I can rename it. But it might affect a lot of files :)
// this would normally submitted by releayer. It call the subnet actor on the L2 network. | ||
submitBottomUpCheckpoint( | ||
callCreateBottomUpCheckpointFromChildSubnet(params.subnetL3.id, params.subnetL3.gateway), | ||
params.subnetL3.subnetActor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the params.subnetL3.subnetActor
refers to the SubnetActorDiamond
in L2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
} | ||
|
||
function createNativeSubnet( | ||
address[] memory subnetPath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt this be parent subnet id? How do you know the subnet path before it's created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is supposed to be the parent subnet path, but it seems redundant since we can just get it from parentId.path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch. The parent id route can be used.
@karlem Another question I have is regarding nonce. Say a bottom up message is created in L3, the nonce will be set by the local network. Then in L2, will the cross network message nonce change? If so, then the id ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments.
revert NotRegisteredSubnet(); | ||
} | ||
|
||
function commitTopDownMsg(Subnet storage subnet, IpcEnvelope memory crossMessage) internal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. This also feels unsafe since this function could accidentally modify the subnet state, since it's passed with the storage
modifier.
@@ -404,11 +444,20 @@ library LibGateway { | |||
// should increase the appliedNonce to allow the execution of the next message | |||
// of the batch (this is way we have this after the nonce logic). | |||
if (!crossMsg.to.subnetId.equals(s.networkName)) { | |||
CrossMessageValidationOutcome outcome = validateCrossMessage(crossMsg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cryptoAtwill unfortunately Solidity's try/catch is profoundly crippled because it only works for cross-contract calls.
@@ -226,3 +226,31 @@ contract MockIpcContractPayable is IIpcHandler { | |||
|
|||
receive() external payable {} | |||
} | |||
|
|||
contract MockContractFailbackable { | |||
// receive() external payable {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to test this with receive as well?
IPCAddress({ | ||
subnetId: gatewayDiamond.getter().getNetworkName().getParentSubnet(), | ||
rawAddress: FvmAddressHelper.from(receipient) | ||
}), | ||
IPCAddress({subnetId: id, rawAddress: FvmAddressHelper.from(address(this))}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it was labeled as a top-down message, but the path (from, to) made it a bottom-up message, the test was incorrect. Since we now have checks ensuring that top-down messages are indeed top-down (as a parent can't send a message to itself), this would cause a revert.
}); | ||
|
||
address[] memory nativeSubnetPath; | ||
nativeSubnet = createNativeSubnet(nativeSubnetPath, rootNetwork.gatewayAddr, rootNetwork.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to pass an empty address[]
as the first argument?
} | ||
|
||
function createNativeSubnet( | ||
address[] memory subnetPath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is supposed to be the parent subnet path, but it seems redundant since we can just get it from parentId.path?
Hmm that's a good point. I will think about how to mitigate this. |
Part of #1080
This PR focuses on:
Integration Tests in Solidity (L3-Level Testing)
Improvements
CheckpointCommitted
NewBottomUpMessage
MessageStoredInPostbox
MessagePropagatedFromPostbox