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: add ARC22 and ARC28 interfaces for ABI contracts and methods #856

Merged
merged 4 commits into from
Jul 29, 2024

Conversation

joe-p
Copy link
Contributor

@joe-p joe-p commented Feb 17, 2024

Adds ARC28 (events) and ARC22 (readonly) to ABI-related interfaces

@joe-p joe-p changed the title feat: add ARC22 readonly to ABIMethodParams feat: add ARC22 and ARC28 interfaces for ABI contracts and methods Apr 11, 2024
@joe-p joe-p requested a review from a team as a code owner April 11, 2024 15:40
@joe-p joe-p requested a review from onetechnical April 11, 2024 15:40
@joe-p

This comment was marked as outdated.

@pbennett
Copy link

pbennett commented May 2, 2024

Just chiming in to say this would be nice to get in - it's preventing me from cleanly including ARC28 events in my tealscript contracts. The generated json blocks tests from being run because it doesn't match the schema defined in the sdk.

@onetechnical onetechnical requested review from jasonpaulos and removed request for onetechnical May 8, 2024 14:31
src/abi/contract.ts Show resolved Hide resolved
src/abi/method.ts Show resolved Hide resolved
src/abi/event.ts Show resolved Hide resolved
src/abi/interface.ts Outdated Show resolved Hide resolved
src/abi/method.ts Outdated Show resolved Hide resolved
@jasonpaulos
Copy link
Contributor

jasonpaulos commented May 31, 2024

Also it looks like this failing in cucumber tests:

13) Scenario: Serialize Contract into json # tests/cucumber/features/unit/abijson.feature:97
    ✔ When I create the Method object from method signature "add(uint32,uint32)uint32" # tests/cucumber/steps/index.js:510
    ✔ And I create a Contract object from the Method object with name "ExampleContract" and description "This is an example contract" # tests/cucumber/steps/index.js:510
    ✔ And I set the Contract's appID to 1234 for the network "wGHE2Pwdvd7S12BL5FaOP20EGYesN73ktiC1qzkkit8=" # tests/cucumber/steps/index.js:510
    ✔ And I set the Contract's appID to 5678 for the network "SGO1GKSzyE7IEPItTxCByw9x8FmnrCDexi9/cOUJOiI=" # tests/cucumber/steps/index.js:510
    ✔ And I serialize the Contract object into json # tests/cucumber/steps/index.js:510
    ✖ Then the produced json should equal "contract.json" loaded from "abi_responsejsons" # tests/cucumber/steps/index.js:579
        AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
        + actual - expected ... Lines skipped

          {
            desc: 'This is an example contract',
        +   events: [],
        +   methods: [
        +     {
        +       args: [
        +         {
        +           type: 'uint32'
        +         },
        +         {
        +           type: 'uint32'
        +         }
        +       ],
        +       events: [],
        -   methods: [
        -     {
        -       args: [
        -         {
        -           type: 'uint32'
        -         },
        -         {
        -           type: 'uint32'
        -         }
        -       ],
                name: 'add',
        ...
              }
            }
          }

Perhaps when converting to JSON, if no events are present, that field should be omitted from the result? Since these tests run on every SDK, they're a bit difficult to deal with.

@joe-p
Copy link
Contributor Author

joe-p commented Jun 10, 2024

I made readonly and events optional so if they weren't provided upon instantiation they won't be included in the JSON output.

CI seems to be failing now for an unrelated reason:

fatal: Remote branch V2 not found in upstream origin

@pbennett
Copy link

pbennett commented Jul 3, 2024

ping

@bwmx
Copy link

bwmx commented Jul 3, 2024

ping, this missing functionality is making it difficult for new users to get onboarded to algorand development, they get presented with typescript errors (that are difficult for a new user to resolve). It's undoubtedly frustrating for anyone else using the standard toolsets provided for development, unelegant hacks and workaround are the current solution.

@pbennett
Copy link

@jasonpaulos ?

Copy link
Contributor

@jasonpaulos jasonpaulos 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. I was having trouble deciding between whether the new properties should be optional on ABIMethod and ABIContract, like they are in this PR, or if they should be non-optional ARC28Event[] and boolean types, which get coerced from undefined to a default value.

However, I now believe there is value in keeping the types optional, since a value of undefined can indicate that no information is available about these extension fields. For example, I think it's beneficial to be able to differentiate between a contract that claims it emits no events and one that claims nothing at all about events.

Thanks again @joe-p for this contribution

@jasonpaulos jasonpaulos merged commit 7f2e734 into algorand:develop Jul 29, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants