feat(node): support l2 plus value transfer#1240
feat(node): support l2 plus value transfer#1240cryptoAtwill merged 11 commits intosupport-l2-plusfrom
Conversation
raulk
left a comment
raulk
left a comment
There was a problem hiding this comment.
LGTM. For posterity purposes, this is our guiding design sketch:
| } | ||
|
|
||
| function equals(Asset memory asset, Asset memory asset2) internal pure returns (bool) { | ||
| return asset.tokenAddress == asset2.tokenAddress; |
There was a problem hiding this comment.
Let's compare kinds too to make this code more defensive against future changes, e.g. we may introduce a third asset class identified only by kind, or by attributes other than tokenAddress.
| CannotSendToItself, | ||
| CommonParentNotExist | ||
| CommonParentNotExist, | ||
| InvalidSupplySource |
There was a problem hiding this comment.
| InvalidSupplySource | |
| IncompatibleSupplySource |
|
|
||
| /// Checks if the incoming and outgoing subnet supply sources can be mapped. | ||
| /// Caller should make sure the incoming/outgoing subnets and current subnet are immediate parent/child subnets. | ||
| function checkSubnetsAssets( |
There was a problem hiding this comment.
| function checkSubnetsAssets( | |
| function checkSubnetsSupplyCompatible( |
| } | ||
| } | ||
|
|
||
| bool validSupplySources = checkSubnetsAssets({ |
There was a problem hiding this comment.
| bool validSupplySources = checkSubnetsAssets({ | |
| bool supplySourcesCompatible = checkSubnetsAssets({ |
|
@raulk New updates:
|
| sendCrossMessageFromChildToParentWithResult(params); | ||
| } | ||
|
|
||
| function testL2PlusSubnet_TokenMixed_SendCrossMessageFromChildToParentWithOkResult() public { |
There was a problem hiding this comment.
these tests are removed because tokenL2 to token L3 xnet value transfer does not work. Updated in: https://github.com/consensus-shipyard/ipc/pull/1240/files#diff-53695b3660cd8c2d5418a61e61a1abda1cd2ba05dcc0f607ddd3251b9cc8f57dR436
There was a problem hiding this comment.
Removed temporarily, since we cover this error scenario in the cited PR.
| if (crossMsg.kind == IpcMsgKind.Transfer) { | ||
| // If the cross msg kind is Result, create result message should have handled the value correctly. | ||
| // If the execution is ok, value should be 0, else one should perform refund. | ||
| if (crossMsg.kind == IpcMsgKind.Transfer || crossMsg.kind == IpcMsgKind.Result) { |
There was a problem hiding this comment.
This is not correct; the version in main is more correct. For a Result message, you will want to perform a call as this returns control back to the caller. Rationale:
- if it's an account, there will be no code to invoke, so this will be have like a bare transfer
- but if the original caller was a contract, you will want to give it control so it can handle the result
raulk
left a comment
raulk
left a comment
There was a problem hiding this comment.
LGTM, feel free to merge, just a few nits and comments inline.
| sendCrossMessageFromChildToParentWithResult(params); | ||
| } | ||
|
|
||
| function testL2PlusSubnet_TokenMixed_SendCrossMessageFromChildToParentWithOkResult() public { |
There was a problem hiding this comment.
Removed temporarily, since we cover this error scenario in the cited PR.
| function validateCrossMessage(IpcEnvelope memory envelope) internal view returns (CrossMessageValidationOutcome) { | ||
| GatewayActorStorage storage s = LibGatewayActorStorage.appStorage(); | ||
| SubnetID memory toSubnetId = envelope.to.subnetId; | ||
| (CrossMessageValidationOutcome outcome, ) = checkCrossMessage(envelope); | ||
| return outcome; | ||
| } | ||
|
|
||
| /// @notice Validates a cross message and returns the applyType if the message is valid | ||
| function checkCrossMessage(IpcEnvelope memory envelope) internal view returns (CrossMessageValidationOutcome, IPCMsgType applyType) { |
There was a problem hiding this comment.
nit: I would keep these as a single function returning a tuple. Would call this single method inspect().
| // Call flow tests. | ||
| //--------------------- | ||
|
|
||
| // testing Native L1 => ERC20 L2 => ERC20 L3, this supply source is not allowed |
There was a problem hiding this comment.
Can't we add assertions on the returned result here? (i.e. that it's a failure and it carries the correct reason)
| interface IGateway { | ||
| function sendContractXnetMessage( | ||
| IpcEnvelope memory envelope | ||
| ) external payable returns (IpcEnvelope memory committed); | ||
|
|
||
| function fund(SubnetID calldata subnetId, FvmAddress calldata to) external payable; | ||
|
|
||
| function fundWithToken(SubnetID calldata subnetId, FvmAddress calldata to, uint256 amount) external; | ||
| } |
There was a problem hiding this comment.
Do we need this local interface definition here? Can't we import the canonical one from interfaces/?
| // ======= internal util methods ======== | ||
|
|
||
| function executeTopdownMessages(IpcEnvelope[] memory msgs, GatewayDiamond gw) internal { | ||
| uint256 minted_tokens; |
There was a problem hiding this comment.
Convention is lower snake case.
| for (uint256 i; i < msgs.length; ) { | ||
| minted_tokens += msgs[i].value; | ||
| unchecked { | ||
| ++i; | ||
| } | ||
| } |
There was a problem hiding this comment.
Since tests do not run in a gas restricted environment, we can unclutter this by removing the unchecked increment.
| function createBottomUpCheckpoint( | ||
| SubnetID memory subnet, | ||
| GatewayDiamond gw | ||
| ) internal returns (BottomUpCheckpoint memory checkpoint) { | ||
| uint256 e = getNextEpoch(block.number, DEFAULT_CHECKPOINT_PERIOD); | ||
|
|
||
| GatewayGetterFacet getter = gw.getter(); | ||
| CheckpointingFacet checkpointer = gw.checkpointer(); | ||
|
|
||
| BottomUpMsgBatch memory batch = getter.bottomUpMsgBatch(e); | ||
|
|
||
| (, address[] memory addrs, uint256[] memory weights) = TestUtils.getFourValidators(vm); | ||
|
|
||
| (bytes32 membershipRoot, ) = MerkleTreeHelper.createMerkleProofsForValidators(addrs, weights); | ||
|
|
||
| checkpoint = BottomUpCheckpoint({ | ||
| subnetID: subnet, | ||
| blockHeight: batch.blockHeight, | ||
| blockHash: keccak256("block1"), | ||
| nextConfigurationNumber: 0, | ||
| msgs: batch.msgs, | ||
| activity: ActivityHelper.newCompressedActivityRollup(1, 3, bytes32(uint256(0))) | ||
| }); | ||
|
|
||
| vm.prank(FilAddress.SYSTEM_ACTOR); | ||
| checkpointer.createBottomUpCheckpoint( | ||
| checkpoint, | ||
| membershipRoot, | ||
| weights[0] + weights[1] + weights[2], | ||
| ActivityHelper.dummyActivityRollup() | ||
| ); | ||
|
|
||
| return checkpoint; | ||
| } |
There was a problem hiding this comment.
Do we really not have utils for this kind of common stuff in a test library?
There was a problem hiding this comment.
not really, I created an issue to track this good to have: #1244
Co-authored-by: cryptoAtwill <willes.lau@protocol.ai>
2 participants
This PR builds on previous PR but instead enables the value transfer in cross network messages.
Update 27/12/24: