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

Add assertions to generic transfer and update docs #166

Merged
merged 7 commits into from
Sep 9, 2024
Merged

Conversation

joshuahannan
Copy link
Member

@joshuahannan joshuahannan commented Jul 9, 2024

Closes: #162

Description

  • Adds assertions to generic_transfer_with_address to make sure the vault is the correct type
  • Added a test with MaliciousToken to test the transaction. MaliciousToken is the same as ExampleToken, but returns the ExampleToken paths in the FTVaultData view instead of its own paths.
  • Adds FLIX to the main generic transactions
  • Updates comments in FungibleToken
  • Updates tests to use the pre-deployed versions of the contracts that do not come from this repo instead of deploying new ones

I can't think of any other assertions to put in the transactions, but I am open to suggestions


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@joshuahannan joshuahannan added enhancement New feature or request Improvement Technical Debt SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board labels Jul 9, 2024
Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this review. Left one non-blocking comment

if addressString.length == 18 {
addressString = addressString.slice(from: 2, upTo: 18)
}
let typeString: String = "A.".concat(addressString).concat(".").concat(contractName).concat(".Vault")
Copy link
Contributor

Choose a reason for hiding this comment

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

This covers all existing Vault implementations but wouldn't cover multi-vault FT contracts. We should probably note that in the docs and add a generic_transfer_with_identifier as was suggested for bridge transactions

Copy link
Member Author

Choose a reason for hiding this comment

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

good point! I created an issue here:#168

@joshuahannan joshuahannan merged commit 1bbb412 into master Sep 9, 2024
2 checks passed
@joshuahannan joshuahannan deleted the generic-txs branch September 9, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Improvement SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board Technical Debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Put assertions and post-conditions in the generic transactions
2 participants