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

Some tweaks to PR 69 #70

Closed
wants to merge 9 commits into from
Closed

Some tweaks to PR 69 #70

wants to merge 9 commits into from

Conversation

geoknee
Copy link
Contributor

@geoknee geoknee commented Jun 8, 2022

No description provided.

// * an allocations array
struct SingleAssetExit {
address asset;
bytes metadata;
TokenMetadata tokenMetadata;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this is a breaking change. We could maintain backwards compatibility by encoding TokenMetadata into bytes. This would cost some more gas.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is much cleaner and more gas efficient for sure. It is just if a breaking change is viable at this stage. Maybe better to do it sooner rather than later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@willemolding we are comfortable with a breaking change.

TODO: We will want to handle it by increasing the first nonzero portion of the package semver which should prevent the breaking change reaching downstream packages until they explicitly upgrade.

If you like this approach, why not merge this into your PR, and then we can get a final review on #69?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do!

Allocation[] allocations;
}

struct TokenMetadata {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is AssetMetadata better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that is more general

@geoknee geoknee marked this pull request as ready for review June 16, 2022 16:24
@geoknee geoknee closed this Jun 23, 2022
@geoknee geoknee deleted the gkerc1155 branch June 23, 2022 09:55
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.

None yet

2 participants