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: import transactions from Safe builder #147

Conversation

gsteenkamp89
Copy link

@gsteenkamp89 gsteenkamp89 commented Mar 20, 2024

Fixes #UMA-2431

motivation

We want users to be able to use the transaction builder in the safe app, export it, and then import it into the oSnap plugin.

How to test

  1. create a new Proposal (check "use oSnap" checkbox)
  2. Add a new transaction
  3. Choose transaction type "Safe Import" from list
  4. either drag n drop or select file from machine. (see 2 test files below)
  5. there are multiple transactions in the file. choose 1 to import.
  6. There are 2 transaction types Native transfer & contract interaction
  7. The relevant input fileds should be populated with the values from the file and validated (it is possible to export invalid values from safe's builder, so we must validate each input here in the app)

Test files

This file works ✅
test-file-VALID.json

This file is corrupted ❌ (should display an error message)
test-file-INVALID.json

screenshots:

idle state
Screenshot 2024-03-20 at 13 43 15
add file, choose transaction
Screenshot 2024-03-20 at 13 43 31
Native transfer
Screenshot 2024-03-20 at 13 43 58
Contract Interaction (includes erc20 transfers)
Screenshot 2024-03-20 at 13 43 51
If you drag n drop a non-json file
Screenshot 2024-03-20 at 14 12 25
If there is an error while attempting to parse the file. can be syntax errors in the file or the schema does not match what we expect.
Screenshot 2024-03-20 at 14 13 19

Copy link

linear bot commented Mar 20, 2024

@gsteenkamp89 gsteenkamp89 requested a review from daywiss March 20, 2024 14:24
@gsteenkamp89 gsteenkamp89 changed the title Gerhard/uma 2431 figure out how to get safe transactions into snapshot osnap feat: import transactions from Safe builder Mar 20, 2024
Copy link

vercel bot commented Mar 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
snapshot ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 1, 2024 1:48pm
snapshot-goerli ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 1, 2024 1:48pm

@daywiss
Copy link

daywiss commented Mar 25, 2024

will defer reviewing this until changes from lee's suggestions

Copy link

@daywiss daywiss left a comment

Choose a reason for hiding this comment

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

still having an issue with deleting the first transaction in the list:

  1. import valid tx file
  2. remove first tx
  3. see that first tx stays, but second tx is deleted

also should we disable this or let the user overwrite this tx with a new type:
image

error.value = undefined;
}

// TODO: allow multiple files at once
Copy link

Choose a reason for hiding this comment

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

we need to allow multiple files

Copy link
Author

Choose a reason for hiding this comment

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

we are only allowing them to drop 1 file at a time. but they can drop many, one after the other.

throw new Error('Less than 32 bytes');
}

if (!isBytesLike(value)) {
Copy link

Choose a reason for hiding this comment

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

this check should be done last. i think it should either be in this order, too short, too long, isBytesLIke

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@gsteenkamp89
Copy link
Author

still having an issue with deleting the first transaction in the list:

  1. import valid tx file
  2. remove first tx
  3. see that first tx stays, but second tx is deleted

also should we disable this or let the user overwrite this tx with a new type: image

There was some funky localised state polluting things in this flow and causing bugs like this. Now only emitting state changes upstream, much better.

@daywiss
Copy link

daywiss commented Mar 29, 2024

looking good but found another issue. after quick fixing the issues in tx 1, and then trying to simulate, it still says its inavlid:
image

Copy link

@daywiss daywiss left a comment

Choose a reason for hiding this comment

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

looks good. remember dont merge this, please pr to snapshot!

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.

2 participants