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

Redesign abi generation for multi_index and singleton #201

Merged
merged 18 commits into from
Jul 28, 2023

Conversation

dimas1185
Copy link
Contributor

@dimas1185 dimas1185 commented Jul 18, 2023

After allowing non-table types inside eosio::singleton we extended types that can appear in abi. That way non contract singleton can appear in abi.
That PR resolves #198 and defines the following rules:
multi_index and singleton tables together with underlying structures appear in abi only if they are defined or aliased inside main contract class. If abi generator finds any multi_index or singleton outside main contract class without aliasing them inside main contract class (either by typedef or using) - those are ignored and won't appear in contract class.

Change Description

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

@dimas1185 dimas1185 changed the base branch from main to release/4.0 July 24, 2023 21:16
if (!contract_class) {
contract_class = find_contract_class(decl->getASTContext());
if (!contract_class)
CDT_ERROR("abigen_error", decl->getLocation(), "contract class not found");
Copy link
Member

Choose a reason for hiding this comment

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

Will CDT_ERROR throw? Should just report the error and return false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. this one doesn't throw so I changed the macro

Copy link
Member

Choose a reason for hiding this comment

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

It is not an error to not find the contract class, right? Just means this type is not in a contract class.

@dimas1185 dimas1185 changed the base branch from release/4.0 to main July 26, 2023 21:20
tools/include/eosio/abigen.hpp Outdated Show resolved Hide resolved
tools/include/eosio/abigen.hpp Outdated Show resolved Hide resolved
if (!contract_class) {
contract_class = find_contract_class(decl->getASTContext());
if (!contract_class)
CDT_ERROR("abigen_error", decl->getLocation(), "contract class not found");
Copy link
Member

Choose a reason for hiding this comment

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

It is not an error to not find the contract class, right? Just means this type is not in a contract class.

if (!contract_class) {
contract_class = find_contract_class(decl->getASTContext());
if (!contract_class)
CDT_INTERNAL_ERROR("contract class not found");
Copy link
Member

Choose a reason for hiding this comment

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

This is not an error, right? Just means it is not in the main eosio contract.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a fatal error. we must have a contract class that corresponds to c++ filename or --contract option

Copy link
Member

Choose a reason for hiding this comment

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

But will that always be accessible from the current translation unit?

Copy link
Contributor Author

@dimas1185 dimas1185 Jul 27, 2023

Choose a reason for hiding this comment

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

good point.. maybe you are right, we should just return false if no contract.
So this is abi generator. Currently we are not traversing translation units other than containing a main file:
https://github.com/AntelopeIO/cdt/blob/extend-abi-non-contract/tools/include/eosio/abigen.hpp#L878

However if we will change that in any way we may want to just return false. That is fine, if type is defined in main file, it will be picked up anyway.

@greg7mdp greg7mdp removed their request for review July 28, 2023 12:56
@dimas1185 dimas1185 merged commit 0056647 into main Jul 28, 2023
6 checks passed
@dimas1185 dimas1185 deleted the extend-abi-non-contract branch July 28, 2023 18:01
@ericpassmore ericpassmore added this to the CDT 4.0.1 milestone Aug 15, 2023
@dimas1185 dimas1185 mentioned this pull request Sep 19, 2023
2 tasks
@arhag arhag removed this from the CDT 4.0.1 milestone Sep 20, 2023
dimas1185 added a commit that referenced this pull request Sep 22, 2023
@dimas1185 dimas1185 mentioned this pull request Sep 22, 2023
2 tasks
@ericpassmore
Copy link
Contributor

Note:start
group: STABILITY
category: BUG
summary: Update ABI generation for multi_index and singleton to correctly scope types for inclusion.
Note:end

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.

Correct target for Type in ABI
6 participants