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

feat(rfq-relayer): support FastBridgeV2 with arbitrary calls [SLT-320] #3258

Open
wants to merge 45 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
36a42bd
feat: add `callValue` to bridge params
ChiTimesChi Oct 2, 2024
919a0a9
feat: add `callValue` to bridge tx V2
ChiTimesChi Oct 2, 2024
867d5d9
test: add cases for SRC calls with `callValue`
ChiTimesChi Oct 3, 2024
c93c403
refactor: simplify tests further
ChiTimesChi Oct 3, 2024
9575246
test: add cases for DST relays with `callValue`
ChiTimesChi Oct 3, 2024
7872f2a
feat: support `callValue` in `bridge()`
ChiTimesChi Oct 3, 2024
ad7dd4c
feat: support `callValue` in `relay()`
ChiTimesChi Oct 3, 2024
5ca5ad1
test: update revert message for failed ETH transfers
ChiTimesChi Oct 3, 2024
e8355c3
refactor: always forward full msg.value for the hook call
ChiTimesChi Oct 3, 2024
eb2bbb8
refactor: use `_pullToken` only in `bridge()`
ChiTimesChi Oct 3, 2024
c50042a
refactor: isolate validation of relay params
ChiTimesChi Oct 3, 2024
9fb4461
refactor: isolate validation of the bridge params
ChiTimesChi Oct 3, 2024
156e333
docs: could -> can
ChiTimesChi Oct 8, 2024
2b77b4a
test: enable the backwards compatibility encoding test
ChiTimesChi Oct 8, 2024
271f59d
fix: getBridgeTransaction partial support for V2 structs
ChiTimesChi Oct 8, 2024
2317a58
test: add clarifications about expected reverts
ChiTimesChi Oct 8, 2024
c6a1fdc
Feat: initial fastbridgev2 bindings
dwasse Oct 8, 2024
e5e8646
Feat: abigen helpers
dwasse Oct 8, 2024
ab63286
Fix: generate
dwasse Oct 8, 2024
93d9b7d
Fix: bridge enum test
dwasse Oct 8, 2024
e34a08b
Feat: relayer integrates fastbridgev2
dwasse Oct 8, 2024
1056ef1
Feat: testutils uses v2
dwasse Oct 8, 2024
1301270
Fix: deployer uses v2
dwasse Oct 8, 2024
1081e0a
Fix: current e2e tests with v2 bridge
dwasse Oct 8, 2024
467d5c7
Feat: add recipient mock test contract
dwasse Oct 9, 2024
f456bb7
Feat: add TestUSDCtoUSDCWithCallData
dwasse Oct 9, 2024
de621a7
Rename: TestArbitraryCall
dwasse Oct 9, 2024
e3c1604
Feat: add TransactionV1 in QuoteRequest struct for access to SendChai…
dwasse Oct 9, 2024
bff4f72
Cleanup: bridge tx fetching
dwasse Oct 9, 2024
4ed813b
Cleanup: remove guard check for now
dwasse Oct 9, 2024
5b6ec12
Feat: add v2 of fastbridgemock
dwasse Oct 11, 2024
d3dbeb0
Fix: build
dwasse Oct 11, 2024
0bc22c5
Cleanup: lint
dwasse Oct 11, 2024
117ce59
Cleanup: remove unnecessary test
dwasse Oct 11, 2024
6dee3d1
Fix: mock fast bridge deployer
dwasse Oct 11, 2024
f57312b
Revert "Cleanup: remove unnecessary test"
dwasse Oct 11, 2024
4b8dc68
Fix: flatten all files
dwasse Oct 11, 2024
b24d31d
Revert "Fix: flatten all files"
dwasse Oct 11, 2024
91a6b8f
Feat: flatten mocks
dwasse Oct 11, 2024
b6a4609
Fix: tests
dwasse Oct 11, 2024
05ab3dd
Fix: test
dwasse Oct 11, 2024
94ee810
Fix: use nativeTokenDecimals instead of origin decimals for call value
dwasse Oct 15, 2024
337782c
Merge branch 'master' into feat/relayer-arb-call
dwasse Oct 15, 2024
8759318
Update abigen
dwasse Oct 15, 2024
f44c7ae
Fix: use native token decimals for CallValue
dwasse Oct 16, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/contracts-rfq/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"lint": "forge fmt && npm run solhint",
"lint:check": "forge fmt --check && npm run solhint:check",
"ci:lint": "npm run lint:check",
"build:go": "./flatten.sh contracts/*.sol test/*.sol",
"build:go": "./flatten.sh contracts/*.sol test/*.sol test/mocks/*.sol",
"solhint": "solhint '{contracts,script,test}/**/*.sol' --fix --noPrompt --max-warnings 3",
"solhint:check": "solhint '{contracts,script,test}/**/*.sol' --max-warnings 3"
}
Expand Down
2 changes: 2 additions & 0 deletions services/rfq/contracts/fastbridge/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ func (b BridgeStatus) Int() uint8 {
}

// set all contact types.
//
//nolint:gosec,intrange
func init() {
for i := 0; i < len(_BridgeStatus_index)-1; i++ {
contractType := BridgeStatus(i)
Expand Down
27 changes: 27 additions & 0 deletions services/rfq/contracts/fastbridgev2/bridgestatus_string.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

82 changes: 82 additions & 0 deletions services/rfq/contracts/fastbridgev2/events.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package fastbridgev2

import (
"bytes"
"strings"

"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/common"
)

// TODO: consider not exporting to avoid accidental mutation.
var (
// BridgeRequestedTopic is the event topic for a bridge request.
BridgeRequestedTopic common.Hash
// BridgeRelayedTopic is the topic emitted by a bridge relay.
BridgeRelayedTopic common.Hash
// BridgeProofProvidedTopic is the topic emitted by a bridge relay.
BridgeProofProvidedTopic common.Hash
// BridgeDepositClaimedTopic is the topic emitted by a bridge relay.
BridgeDepositClaimedTopic common.Hash
// BridgeProofDisputedTopic is the topic emitted by a bridge dispute.
BridgeProofDisputedTopic common.Hash
)

// static checks to make sure topics actually exist.
func init() {
var err error

parsedABI, err := abi.JSON(strings.NewReader(FastBridgeV2MetaData.ABI))
if err != nil {
panic(err)

Check warning on line 31 in services/rfq/contracts/fastbridgev2/events.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/contracts/fastbridgev2/events.go#L31

Added line #L31 was not covered by tests
}

BridgeRequestedTopic = parsedABI.Events["BridgeRequested"].ID
BridgeRelayedTopic = parsedABI.Events["BridgeRelayed"].ID
BridgeProofProvidedTopic = parsedABI.Events["BridgeProofProvided"].ID
BridgeDepositClaimedTopic = parsedABI.Events["BridgeDepositClaimed"].ID
BridgeProofDisputedTopic = parsedABI.Events["BridgeProofDisputed"].ID
Comment on lines +34 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle missing events to prevent potential runtime panics

When accessing events in parsedABI.Events, if an event key like "BridgeRequested" does not exist, it will cause a runtime panic due to a nil pointer dereference. It's safer to check if the event exists before accessing its ID.

Apply this change to handle missing events gracefully:

-BridgeRequestedTopic = parsedABI.Events["BridgeRequested"].ID
+event, exists := parsedABI.Events["BridgeRequested"]
+if !exists {
+    panic("BridgeRequested event not found in ABI")
+}
+BridgeRequestedTopic = event.ID

-BridgeRelayedTopic = parsedABI.Events["BridgeRelayed"].ID
+event, exists = parsedABI.Events["BridgeRelayed"]
+if !exists {
+    panic("BridgeRelayed event not found in ABI")
+}
+BridgeRelayedTopic = event.ID

// Repeat for the other events...

This ensures that if an event is missing, a clear panic message is provided, aiding in debugging.

Committable suggestion was skipped due to low confidence.


_, err = parsedABI.EventByID(BridgeRequestedTopic)
if err != nil {
panic(err)

Check warning on line 42 in services/rfq/contracts/fastbridgev2/events.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/contracts/fastbridgev2/events.go#L42

Added line #L42 was not covered by tests
}

_, err = parsedABI.EventByID(BridgeRelayedTopic)
if err != nil {
panic(err)

Check warning on line 47 in services/rfq/contracts/fastbridgev2/events.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/contracts/fastbridgev2/events.go#L47

Added line #L47 was not covered by tests
}

_, err = parsedABI.EventByID(BridgeProofProvidedTopic)
if err != nil {
panic(err)

Check warning on line 52 in services/rfq/contracts/fastbridgev2/events.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/contracts/fastbridgev2/events.go#L52

Added line #L52 was not covered by tests
}

_, err = parsedABI.EventByID(BridgeProofDisputedTopic)
if err != nil {
panic(err)

Check warning on line 57 in services/rfq/contracts/fastbridgev2/events.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/contracts/fastbridgev2/events.go#L57

Added line #L57 was not covered by tests
}
}
Comment on lines +54 to +59
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Missing validation for BridgeDepositClaimedTopic in init() function

The init() function validates the existence of four event topics using parsedABI.EventByID, but it omits validation for BridgeDepositClaimedTopic. To ensure all event topics are properly validated, consider adding the missing validation.

Apply this diff to include the missing validation:

 	_, err = parsedABI.EventByID(BridgeProofProvidedTopic)
 	if err != nil {
 		panic(err)
 	}

+	// Validate BridgeDepositClaimedTopic
+	_, err = parsedABI.EventByID(BridgeDepositClaimedTopic)
+	if err != nil {
+		panic(err)
+	}

 	_, err = parsedABI.EventByID(BridgeProofDisputedTopic)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
_, err = parsedABI.EventByID(BridgeProofDisputedTopic)
if err != nil {
panic(err)
}
}
_, err = parsedABI.EventByID(BridgeProofProvidedTopic)
if err != nil {
panic(err)
}
// Validate BridgeDepositClaimedTopic
_, err = parsedABI.EventByID(BridgeDepositClaimedTopic)
if err != nil {
panic(err)
}
_, err = parsedABI.EventByID(BridgeProofDisputedTopic)
if err != nil {
panic(err)
}
}


// topicMap maps events to topics.
// this is returned as a function to assert immutability.
func topicMap() map[EventType]common.Hash {
return map[EventType]common.Hash{
BridgeRequestedEvent: BridgeRequestedTopic,
BridgeRelayedEvent: BridgeRelayedTopic,
BridgeProofProvidedEvent: BridgeProofProvidedTopic,
BridgeDepositClaimedEvent: BridgeDepositClaimedTopic,
BridgeDisputeEvent: BridgeProofDisputedTopic,
}

Check warning on line 70 in services/rfq/contracts/fastbridgev2/events.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/contracts/fastbridgev2/events.go#L63-L70

Added lines #L63 - L70 were not covered by tests
}
Comment on lines +61 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure immutability of the map returned by topicMap

The topicMap function currently returns a new map every time it's called. This can lead to unnecessary memory allocations and potential performance issues.

Consider returning a reference to a pre-defined, immutable map:

  1. Define the map as a package-level variable:

    var eventTypeToTopic = map[EventType]common.Hash{
        BridgeRequestedEvent:      BridgeRequestedTopic,
        BridgeRelayedEvent:        BridgeRelayedTopic,
        BridgeProofProvidedEvent:  BridgeProofProvidedTopic,
        BridgeDepositClaimedEvent: BridgeDepositClaimedTopic,
        BridgeDisputeEvent:        BridgeProofDisputedTopic,
    }
  2. Modify topicMap to return the pre-defined map:

    -func topicMap() map[EventType]common.Hash {
    -    return map[EventType]common.Hash{
    -        // ...
    -    }
    +func topicMap() map[EventType]common.Hash {
    +    return eventTypeToTopic
    }

    Alternatively, you might consider removing the topicMap function entirely and accessing eventTypeToTopic directly, if appropriate.


// eventTypeFromTopic gets the event type from the topic
// returns nil if the topic is not found.
func eventTypeFromTopic(ogTopic common.Hash) *EventType {
for eventType, topic := range topicMap() {
if bytes.Equal(ogTopic.Bytes(), topic.Bytes()) {
return &eventType
}

Check warning on line 79 in services/rfq/contracts/fastbridgev2/events.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/contracts/fastbridgev2/events.go#L75-L79

Added lines #L75 - L79 were not covered by tests
}
return nil

Check warning on line 81 in services/rfq/contracts/fastbridgev2/events.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/contracts/fastbridgev2/events.go#L81

Added line #L81 was not covered by tests
}
Comment on lines +75 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize eventTypeFromTopic by caching the topic map

Currently, topicMap() is called each time eventTypeFromTopic is invoked, creating a new map on each call. For improved performance, consider caching the topic map to avoid unnecessary allocations.

Apply this diff to use a cached topic map:

+var cachedTopicMap = topicMap()

 func eventTypeFromTopic(ogTopic common.Hash) *EventType {
-	for eventType, topic := range topicMap() {
+	for eventType, topic := range cachedTopicMap {
 		if bytes.Equal(ogTopic.Bytes(), topic.Bytes()) {
 			return &eventType
 		}
 	}
 	return nil
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func eventTypeFromTopic(ogTopic common.Hash) *EventType {
for eventType, topic := range topicMap() {
if bytes.Equal(ogTopic.Bytes(), topic.Bytes()) {
return &eventType
}
}
return nil
}
var cachedTopicMap = topicMap()
func eventTypeFromTopic(ogTopic common.Hash) *EventType {
for eventType, topic := range cachedTopicMap {
if bytes.Equal(ogTopic.Bytes(), topic.Bytes()) {
return &eventType
}
}
return nil
}

Comment on lines +75 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize eventTypeFromTopic using a map for constant-time lookups

Currently, eventTypeFromTopic performs a linear search over the topics, which can be inefficient as the number of events grows. Consider using a map to achieve O(1) lookup time.

Implement a reverse mapping from common.Hash to EventType:

  1. Create a global variable for the topic-to-event mapping:

    var topicToEventType = map[common.Hash]EventType{
        BridgeRequestedTopic:      BridgeRequestedEvent,
        BridgeRelayedTopic:        BridgeRelayedEvent,
        BridgeProofProvidedTopic:  BridgeProofProvidedEvent,
        BridgeDepositClaimedTopic: BridgeDepositClaimedEvent,
        BridgeProofDisputedTopic:  BridgeDisputeEvent,
    }
  2. Refactor eventTypeFromTopic to use the map:

    -func eventTypeFromTopic(ogTopic common.Hash) *EventType {
    -    for eventType, topic := range topicMap() {
    -        if bytes.Equal(ogTopic.Bytes(), topic.Bytes()) {
    -            return &eventType
    -        }
    -    }
    -    return nil
    +func eventTypeFromTopic(ogTopic common.Hash) EventType {
    +    if eventType, exists := topicToEventType[ogTopic]; exists {
    +        return eventType
    +    }
    +    return UnknownEventType
    }

    This refactor improves performance and simplifies the code.


⚠️ Potential issue

Avoid returning the address of the loop variable eventType

In the eventTypeFromTopic function, returning the address of the loop variable eventType can lead to unexpected behavior. In Go, the loop variable in a for range is reused in each iteration, so all returned pointers would point to the same memory address, potentially causing incorrect results.

Apply this diff to return the value instead of a pointer:

-func eventTypeFromTopic(ogTopic common.Hash) *EventType {
+func eventTypeFromTopic(ogTopic common.Hash) EventType {
     for eventType, topic := range topicMap() {
         if bytes.Equal(ogTopic.Bytes(), topic.Bytes()) {
-            return &eventType
+            return eventType
         }
     }
-    return nil
+    return UnknownEventType
 }

Ensure that UnknownEventType is defined to represent an invalid or unknown event type.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func eventTypeFromTopic(ogTopic common.Hash) *EventType {
for eventType, topic := range topicMap() {
if bytes.Equal(ogTopic.Bytes(), topic.Bytes()) {
return &eventType
}
}
return nil
}
func eventTypeFromTopic(ogTopic common.Hash) EventType {
for eventType, topic := range topicMap() {
if bytes.Equal(ogTopic.Bytes(), topic.Bytes()) {
return eventType
}
}
return UnknownEventType
}

28 changes: 28 additions & 0 deletions services/rfq/contracts/fastbridgev2/eventtype_string.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions services/rfq/contracts/fastbridgev2/export_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package fastbridgev2

// GetAllBridgeStatuses exports all bridge statuses for testing.
func GetAllBridgeStatuses() []BridgeStatus {
return allBridgeStatuses
}
Loading
Loading