-
Notifications
You must be signed in to change notification settings - Fork 408
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: execution method for creation + batches #1924
Conversation
Branch preview✅ Deploy successful! https://batch_creation_method--webcore.review-web-core.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
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.
Looks good to me. Left a few comments though
@@ -35,12 +35,14 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe | |||
const { maxFeePerGas, maxPriorityFeePerGas } = useGasPrice() | |||
const saltNonce = useMemo(() => Date.now(), []) | |||
const [_, setPendingSafe] = useLocalStorage<PendingSafeData | undefined>(SAFE_PENDING_CREATION_STORAGE_KEY) | |||
const [executionMethod, setExecutionMethod] = useState(ExecutionMethod.RELAY) | |||
|
|||
const ownerAddresses = useMemo(() => data.owners.map((owner) => owner.address), [data.owners]) | |||
const [minRelays] = useLeastRemainingRelays(ownerAddresses) | |||
|
|||
// Chain supports relaying and relay transactions are available |
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 comment is outdated I think. If we decide to keep it I suggest
"Every owner address has remaining relay transactions and relay method is selected"
</Typography> | ||
{!noLabel ? ( | ||
<Typography variant="body2" className={css.label}> | ||
Choose execution method |
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.
Choose execution method | |
Choose execution method: |
// Chain has relaying feature and available relays | ||
const willRelay = relays && relays.remaining > 0 | ||
const canRelay = relays && relays.remaining > 0 | ||
const willRelay = canRelay && executionMethod === ExecutionMethod.RELAY |
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.
Time to make an util for this logic? It will become less prone to inconsistencies
LGTM. Tried it in the GNO chain Tried safe creation with the relayer, with the wallet and run out of relayer funds where you have to pay with your wallet. |
@iamacook please check the unit tests, something got changed after pulling from dev. |
What it solves
Resolves #1894 (Safe creation and batch transactions)
How this PR fixes it
The execution method selector has been added to the review step of the Safe creation flow, as well as the batch transaction modal.
How to test it
Create a Safe and observe the execution method in the review step:
Create a batch and observe the execution method in the modal:
Note: standard transactions should work as before: relay and standard executions.
Screenshots
Safe creation
Batch execution
Checklist