-
Notifications
You must be signed in to change notification settings - Fork 734
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
node: Extend flow cancel by configuring allow-list of chain ID pairs ("pipes") #4229
base: main
Are you sure you want to change the base?
node: Extend flow cancel by configuring allow-list of chain ID pairs ("pipes") #4229
Conversation
4303203
to
35e937b
Compare
54d16d7
to
2c1ba16
Compare
node/pkg/governor/governor_test.go
Outdated
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.
Could you add a test for the flow_cancel_pipes.go
file? Would be good to validate the mainnet list of pipes before deployment ideally, even though we have the runtime checks
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.
Thanks for adding! I was also thinking that it would be good to check every entry to make sure it's a valid pipe and that there are no duplicate pipes
- Add a concept of "pipes" which consist of a pair of chains - Add field in the Governer that enables flow cancel only for "pipes" which are explicitly enabled - Set the Eth-Sui pipe as the only flow cancel-enabled pair. Sui is the only chain with frequent governor congestion so it is the only one that really needs the Flow Cancel capabilities. Ethereum is the main chain that Sui is interacting with in terms of volume moving in and out of Sui. - Refactor calling sites for flow cancel transfer functions so that all checks are done in one place. (Should help prevent bugs where only one calling site is updated.)
- Add unit tests to ensure that the flow cancel feature works iff valid pipes are configured - Adds unit tests for the pipe type (validity, equality between two pipes) - Modifies pipe.equals() to always return false for invalid pipes
037c0ad
to
a24d86d
Compare
node/pkg/governor/governor.go
Outdated
gov.logger.Warn("Error when attempting to add a flow cancel transfer", | ||
zap.Error(err), | ||
) | ||
return false, 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.
Same question for this early return, do we want this? And should the newTransferFromDbTransfer
come before the StoreTransfer
call?
No description provided.