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

Integrated new contracts #322

Merged
merged 6 commits into from
Aug 16, 2024
Merged

Conversation

iulianpascalau
Copy link
Contributor

@iulianpascalau iulianpascalau commented Aug 14, 2024

  • integrated new contracts
  • code cleanup

Reference PR: multiversx/mx-bridge-eth-sc-rs#188

parsers/types.go Outdated Show resolved Hide resolved
args := createMockExecutorArgs()
providedNonce := uint64(8346)
depositNonce := uint64(100)
depositData := string([]byte{bridgeCore.MissingDataProtocolMarker}) + "testData"
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you also add the MissingDataProtocolMarker, this should not be added when calling the deposit function on solidity, only the actual data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just for test purposes, this will emphasize the fact that anything except the missing marker will be wrapped. on L466 (same test) we have this code:

   expectedDepositData := []byte{bridgeCore.DataPresentProtocolMarker, 0, 0, 0, byte(len(depositData))}
   expectedDepositData = append(expectedDepositData, []byte(depositData)...)

the expectedDepositData is 0x01 0x00 0x00 0x00 0x09 01 't' 'e' 's' 't' 'D' 'a' 't' 'a'

if dataLen == 0 {
return
}
if dataLen == 1 && buff[0] == bridgeCore.MissingDataProtocolMarker {
Copy link
Contributor

Choose a reason for hiding this comment

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

on solidity we should not specify the MissingDataProtocolMarker, that and the size should be added by the relayers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will preserve this, as agreed, found the bug on L500, thus. Added integration test, also

sstanculeanu
sstanculeanu previously approved these changes Aug 14, 2024
transfer.DisplayableData = ""
return
}
if dataLen == 1 && buff[0] == bridgeCore.MissingDataProtocolMarker {
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
if dataLen == 1 && buff[0] == bridgeCore.MissingDataProtocolMarker {
// this check is optional, but brings an optimisation to reduce the gas used in case of a bad callData
if dataLen == 1 && buff[0] == bridgeCore.MissingDataProtocolMarker {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@dragos-rebegea dragos-rebegea merged commit 216db92 into feat/v3 Aug 16, 2024
4 checks passed
@dragos-rebegea dragos-rebegea deleted the integrate-new-contracts-2024.08.13 branch August 16, 2024 12:12
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.

3 participants