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 GTF_OUTPUT_COIN_ASSET_ID and GTF_OUTPUT_COIN_TO opcode getters #4694

Merged
merged 11 commits into from
Jun 22, 2023

Conversation

bitzoic
Copy link
Member

@bitzoic bitzoic commented Jun 20, 2023

Description

Added getters for the GTF_OUTPUT_COIN_ASSET_ID and GTF_OUTPUT_COIN_TO opcodes. These have been highly requested by external teams for use in predicates.

Note: The output_asset_to() getter currently returns a b256 rather than an Identity. This should be updated when #4569 is resolved.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@bitzoic bitzoic added the lib: std Standard library label Jun 20, 2023
@bitzoic bitzoic self-assigned this Jun 20, 2023
@bitzoic bitzoic linked an issue Jun 20, 2023 that may be closed by this pull request
@bitzoic bitzoic added the enhancement New feature or request label Jun 20, 2023
@bitzoic bitzoic marked this pull request as ready for review June 20, 2023 19:56
@bitzoic bitzoic requested a review from a team June 20, 2023 19:56
Copy link
Member

@K1-R1 K1-R1 left a comment

Choose a reason for hiding this comment

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

Changes seem fine, but CI is failing

@bitzoic bitzoic requested review from K1-R1, Braqzen, IGI-111 and a team June 22, 2023 00:16
@bitzoic bitzoic added the breaking May cause existing user code to break. Requires a minor or major release. label Jun 22, 2023
K1-R1
K1-R1 previously approved these changes Jun 22, 2023
Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

  1. In the setup the wallet transfers 2 assets. Do we need to transfer the second asset to the predicate prior to testing or can that transfer be removed?
  2. When unpacking variables from the setup function in the tests I would use the same variable names throughout instead of sometimes using wallet and then wallet1 etc.

@bitzoic
Copy link
Member Author

bitzoic commented Jun 22, 2023

  1. In the setup the wallet transfers 2 assets. Do we need to transfer the second asset to the predicate prior to testing or can that transfer be removed?
  2. When unpacking variables from the setup function in the tests I would use the same variable names throughout instead of sometimes using wallet and then wallet1 etc.
  1. This is a setup function. It is setting up. As more GTFopcodes come online this will be needed.
  2. This is done in tests to purposely create distinctions. Nothing is more frustrating than seeing a test fail and not know why because all the variables are named the same.

@bitzoic bitzoic requested review from Braqzen and a team June 22, 2023 16:57
@Braqzen
Copy link
Contributor

Braqzen commented Jun 22, 2023

  1. In the setup the wallet transfers 2 assets. Do we need to transfer the second asset to the predicate prior to testing or can that transfer be removed?
  2. When unpacking variables from the setup function in the tests I would use the same variable names throughout instead of sometimes using wallet and then wallet1 etc.
  1. This is a setup function. It is setting up. As more GTFopcodes come online this will be needed.
  2. This is done in tests to purposely create distinctions. Nothing is more frustrating than seeing a test fail and not know why because all the variables are named the same.
  1. It's a bad practice to add code in anticipation of future features unless you are about to work on those features soon because it creates technical debt as priorities change and the code sits there indefinitely. As things change there is more maintenance on top of polluting the current PR with code that does not serve a function (and thus it should be added later).
  2. It's fine to have different names. I'm referring more towards consistency. For example, the last 2 tests that revert use 2 different variable names for the asset. In the top test the 2nd variable is called asset_id when it should be called asset_id2 thus validating the name asset_id1 in the lower test in the first position. Similarly for any other variable names

@bitzoic
Copy link
Member Author

bitzoic commented Jun 22, 2023

  1. In the setup the wallet transfers 2 assets. Do we need to transfer the second asset to the predicate prior to testing or can that transfer be removed?
  2. When unpacking variables from the setup function in the tests I would use the same variable names throughout instead of sometimes using wallet and then wallet1 etc.
  1. This is a setup function. It is setting up. As more GTFopcodes come online this will be needed.
  2. This is done in tests to purposely create distinctions. Nothing is more frustrating than seeing a test fail and not know why because all the variables are named the same.
  1. It's a bad practice to add code in anticipation of future features unless you are about to work on those features soon because it creates technical debt as priorities change and the code sits there indefinitely. As things change there is more maintenance on top of polluting the current PR with code that does not serve a function (and thus it should be added later).
  2. It's fine to have different names. I'm referring more towards consistency. For example, the last 2 tests that revert use 2 different variable names for the asset. In the top test the 2nd variable is called asset_id when it should be called asset_id2 thus validating the name asset_id1 in the lower test in the first position. Similarly for any other variable names
  1. This is used now, not something in the future.
  2. Updated to asset_id2

@bitzoic bitzoic requested a review from K1-R1 June 22, 2023 17:07
@bitzoic bitzoic requested a review from a team June 22, 2023 17:43
@SwayStar123 SwayStar123 requested review from SwayStar123 and removed request for a team June 22, 2023 17:43
@bitzoic bitzoic merged commit 7245883 into master Jun 22, 2023
@bitzoic bitzoic deleted the bitzoic-4677 branch June 22, 2023 18:29
dmihal added a commit that referenced this pull request May 17, 2024
## Description
When `output_asset_to` was added in #4694, it used `b256` as the return
type, instead of an `Address`. It referenced #4569, which describes the
confusion developers face between `Address` and `Identity`.

While that confusion _is_ an issue, I don't believe that's relevant to
this function type. An output coin can _only_ be an address, not a
contract, since contract balances are stored in the contract UTXO
itself. Furthermore, the standard lib API will need to be frozen soon,
and there's no plans in place to change anything in regards to #4569.

## Checklist

- [ ] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

Co-authored-by: SwayStar123 <46050679+SwayStar123@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking May cause existing user code to break. Requires a minor or major release. enhancement New feature or request lib: std Standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement some commented out GTF opcodes
5 participants