-
Notifications
You must be signed in to change notification settings - Fork 32
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-api): check both relayer role and quoter role #3399
Conversation
WalkthroughThe changes made in this pull request primarily focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.61.0)level=warning msg="[config_reader] The configuration option 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Deploying sanguine-fe with Cloudflare Pages
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/relayer-arb-call #3399 +/- ##
===============================================================
- Coverage 31.14179% 24.77410% -6.36769%
===============================================================
Files 105 208 +103
Lines 4274 13280 +9006
Branches 82 82
===============================================================
+ Hits 1331 3290 +1959
- Misses 2937 9708 +6771
- Partials 6 282 +276
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
services/rfq/api/rest/server.go (1)
Line range hint
366-368
: Improve error message clarity for role verification failure.The error message "relayer not an on-chain relayer" is misleading as it doesn't reflect that we're checking for either quoter or relayer role.
Consider updating the error message:
-return addressRecovered, fmt.Errorf("relayer not an on-chain relayer") +return addressRecovered, fmt.Errorf("address %s does not have required roles (quoter or relayer)", addressRecovered.Hex())services/rfq/e2e/setup_test.go (1)
370-370
: Enhance error messages with more contextThe error messages could be more descriptive to aid in debugging. Consider including the address of the relayer wallet in the error messages.
- return fmt.Errorf("could not get prover role: %w", err) + return fmt.Errorf("could not get prover role for relayer %s: %w", i.relayerWallet.Address(), err) - return fmt.Errorf("could not grant prover role: %w", err) + return fmt.Errorf("could not grant prover role to relayer %s: %w", i.relayerWallet.Address(), err) - return fmt.Errorf("could not get quoter role: %w", err) + return fmt.Errorf("could not get quoter role for relayer %s: %w", i.relayerWallet.Address(), err) - return fmt.Errorf("could not grant quoter role: %w", err) + return fmt.Errorf("could not grant quoter role to relayer %s: %w", i.relayerWallet.Address(), err)Also applies to: 375-375, 381-381, 386-386
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
services/rfq/api/rest/server.go
(1 hunks)services/rfq/e2e/setup_test.go
(1 hunks)
🔇 Additional comments (2)
services/rfq/api/rest/server.go (2)
347-354
: 🛠️ Refactor suggestion
Verify role constant usage and error handling in the role checking logic.
The role checking implementation has the following concerns:
- The role constants are hardcoded strings. Consider defining them as package-level constants to ensure consistency and prevent typos.
- The error handling could be more specific when the quoter role check fails, as it silently falls back to checking the relayer role.
- The error message "unable to check relayer role on-chain" is misleading when checking the quoter role fails.
Consider applying these improvements:
+const (
+ QUOTER_ROLE = "QUOTER_ROLE"
+ RELAYER_ROLE = "RELAYER_ROLE"
+)
-quoterRole := crypto.Keccak256Hash([]byte("QUOTER_ROLE"))
-hasRole, err = bridge.HasRole(ops, quoterRole, addressRecovered)
-if err != nil {
- relayerRole := crypto.Keccak256Hash([]byte("RELAYER_ROLE"))
- hasRole, err = bridge.HasRole(ops, relayerRole, addressRecovered)
- if err != nil {
- return addressRecovered, fmt.Errorf("unable to check relayer role on-chain: %w", err)
- }
-}
+quoterRole := crypto.Keccak256Hash([]byte(QUOTER_ROLE))
+hasRole, err = bridge.HasRole(ops, quoterRole, addressRecovered)
+if err != nil {
+ return addressRecovered, fmt.Errorf("unable to check quoter role on-chain: %w", err)
+}
+if !hasRole {
+ relayerRole := crypto.Keccak256Hash([]byte(RELAYER_ROLE))
+ hasRole, err = bridge.HasRole(ops, relayerRole, addressRecovered)
+ if err != nil {
+ return addressRecovered, fmt.Errorf("unable to check relayer role on-chain: %w", err)
+ }
+}
Let's verify the role constant usage across the codebase:
Line range hint 358-364
: Refine role caching strategy to handle multiple roles.
The current caching implementation has a potential issue: it doesn't distinguish between quoter and relayer roles in the cache key. This could lead to incorrect role verification when an address has one role but not the other.
Consider using role-specific cache keys:
-r.roleCache[destChainID].Set(addressRecovered.Hex(), hasRole, cacheInterval)
+cacheKey := fmt.Sprintf("%s-%s", addressRecovered.Hex(), "QUOTER_ROLE")
+r.roleCache[destChainID].Set(cacheKey, hasRole, cacheInterval)
Let's verify the cache usage:
proverRole, err := rfqContract.PROVERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | ||
if err != nil { | ||
return fmt.Errorf("could not get relayer role: %w", err) | ||
return fmt.Errorf("could not get prover role: %w", err) | ||
} | ||
|
||
tx, err := rfqContract.GrantRole(txContext.TransactOpts, relayerRole, i.relayerWallet.Address()) | ||
tx, err := rfqContract.GrantRole(txContext.TransactOpts, proverRole, i.relayerWallet.Address()) | ||
if err != nil { | ||
return fmt.Errorf("could not grant role: %w", err) | ||
return fmt.Errorf("could not grant prover role: %w", err) | ||
} | ||
backend.WaitForConfirmation(i.GetTestContext(), tx) | ||
|
||
quoterRole, err := rfqContract.QUOTERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | ||
if err != nil { | ||
return fmt.Errorf("could not get quoter role: %w", err) | ||
} | ||
|
||
tx, err = rfqContract.GrantRole(txContext.TransactOpts, quoterRole, i.relayerWallet.Address()) | ||
if err != nil { | ||
return fmt.Errorf("could not grant quoter role: %w", err) |
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.
🛠️ Refactor suggestion
Refactor role granting logic to reduce duplication
The role granting logic is duplicated for both prover and quoter roles. Consider extracting this into a helper function to improve maintainability and reduce code duplication.
+func (i *IntegrationSuite) grantRole(backend backends.SimulatedTestBackend, rfqContract *fastbridge.FastBridge, txContext base.TxContext, roleGetter func(*bind.CallOpts) ([32]byte, error), address common.Address) error {
+ role, err := roleGetter(&bind.CallOpts{Context: i.GetTestContext()})
+ if err != nil {
+ return fmt.Errorf("could not get role for address %s: %w", address, err)
+ }
+
+ tx, err := rfqContract.GrantRole(txContext.TransactOpts, role, address)
+ if err != nil {
+ return fmt.Errorf("could not grant role to address %s: %w", address, err)
+ }
+ backend.WaitForConfirmation(i.GetTestContext(), tx)
+ return nil
+}
func (i *IntegrationSuite) setupRelayer() {
// ...
- proverRole, err := rfqContract.PROVERROLE(&bind.CallOpts{Context: i.GetTestContext()})
- if err != nil {
- return fmt.Errorf("could not get prover role: %w", err)
- }
-
- tx, err := rfqContract.GrantRole(txContext.TransactOpts, proverRole, i.relayerWallet.Address())
- if err != nil {
- return fmt.Errorf("could not grant prover role: %w", err)
- }
- backend.WaitForConfirmation(i.GetTestContext(), tx)
-
- quoterRole, err := rfqContract.QUOTERROLE(&bind.CallOpts{Context: i.GetTestContext()})
- if err != nil {
- return fmt.Errorf("could not get quoter role: %w", err)
- }
-
- tx, err = rfqContract.GrantRole(txContext.TransactOpts, quoterRole, i.relayerWallet.Address())
- if err != nil {
- return fmt.Errorf("could not grant quoter role: %w", err)
- }
- backend.WaitForConfirmation(i.GetTestContext(), tx)
+ if err := i.grantRole(backend, rfqContract, txContext, rfqContract.PROVERROLE, i.relayerWallet.Address()); err != nil {
+ return err
+ }
+
+ if err := i.grantRole(backend, rfqContract, txContext, rfqContract.QUOTERROLE, i.relayerWallet.Address()); err != nil {
+ return err
+ }
// ...
📝 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.
proverRole, err := rfqContract.PROVERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | |
if err != nil { | |
return fmt.Errorf("could not get relayer role: %w", err) | |
return fmt.Errorf("could not get prover role: %w", err) | |
} | |
tx, err := rfqContract.GrantRole(txContext.TransactOpts, relayerRole, i.relayerWallet.Address()) | |
tx, err := rfqContract.GrantRole(txContext.TransactOpts, proverRole, i.relayerWallet.Address()) | |
if err != nil { | |
return fmt.Errorf("could not grant role: %w", err) | |
return fmt.Errorf("could not grant prover role: %w", err) | |
} | |
backend.WaitForConfirmation(i.GetTestContext(), tx) | |
quoterRole, err := rfqContract.QUOTERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | |
if err != nil { | |
return fmt.Errorf("could not get quoter role: %w", err) | |
} | |
tx, err = rfqContract.GrantRole(txContext.TransactOpts, quoterRole, i.relayerWallet.Address()) | |
if err != nil { | |
return fmt.Errorf("could not grant quoter role: %w", err) | |
if err := i.grantRole(backend, rfqContract, txContext, rfqContract.PROVERROLE, i.relayerWallet.Address()); err != nil { | |
return err | |
} | |
if err := i.grantRole(backend, rfqContract, txContext, rfqContract.QUOTERROLE, i.relayerWallet.Address()); err != nil { | |
return err | |
} | |
``` | |
Note: The helper function `grantRole` should be added to the `IntegrationSuite` struct's methods: | |
```go | |
func (i *IntegrationSuite) grantRole(backend backends.SimulatedTestBackend, rfqContract *fastbridge.FastBridge, txContext base.TxContext, roleGetter func(*bind.CallOpts) ([32]byte, error), address common.Address) error { | |
role, err := roleGetter(&bind.CallOpts{Context: i.GetTestContext()}) | |
if err != nil { | |
return fmt.Errorf("could not get role for address %s: %w", address, err) | |
} | |
tx, err := rfqContract.GrantRole(txContext.TransactOpts, role, address) | |
if err != nil { | |
return fmt.Errorf("could not grant role to address %s: %w", address, err) | |
} | |
backend.WaitForConfirmation(i.GetTestContext(), tx) | |
return nil | |
} |
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.
Highlighting a race condition for checking roles
services/rfq/api/rest/server.go
Outdated
g.Go(func() error { | ||
v1Addr, v1Err = r.checkRole(c, destChainID, true) | ||
v1Addr, v1Err = r.checkRole(c, destChainID, true, quoterRole) | ||
return v1Err | ||
}) | ||
g.Go(func() error { | ||
v1Addr, v1Err = r.checkRole(c, destChainID, true, relayerRole) | ||
return v1Err | ||
}) |
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 cache is contract-specific, but not binded to a role. So what likely to happen here is that we check for two different roles in the same contract, one check will result in the actual RPC call, while the second one will just reuse the cached value. Since we don't really need to check multiple roles in the same contract, I suggest to leave the role cache as is. Instead, let's only check for RELAYER_ROLE
in V1 and for QUOTER_ROLE
in V2. This also helps with the future cleanup once the V1 is fully deprecated.
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 could also lead to race conditions between two go routines
Summary by CodeRabbit
New Features
quoterRole
.Bug Fixes
Documentation