-
Notifications
You must be signed in to change notification settings - Fork 6
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: api fungibles pallet #113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions on initial structuring to make it easier to reason and start out closer to how we may end up.
72e6dbc
to
c8d8b2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shaping up, still a decent amount of comments sadly.
Also annoying to have to re-review changes which have already been merged to main, making this take much longer than expected.
/// Returns `Ok(())` if successful, or an error if the transfer fails. | ||
#[pallet::call_index(0)] | ||
#[pallet::weight(AssetsWeightInfo::<T>::transfer_keep_alive())] | ||
pub fn transfer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important question: what happens when v1 needed. Is the plan to create a whole new pallet, taking up an index in the runtime, or to just add new functions here?
If the latter, should these functions be prefixed with v0_ to indicate intentions? Whilst renaming wont affect encoding based on call index, it may break integrations using the name obtained via metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is hard to predict but this theoretically doesn't have to change, ever. As long as the standards are being used the api in the contract should never change. If parameter types change slightly, these should be handled in the pallet via a runtime upgrade but this won't require a different version because the type in the contract stays the same.
When a parameter gets added to pallet assets' transfer
there is a list of things that will happen:
- For old contracts nothing will change, the pallet fungibles'
transfer
will add some logic via a runtime upgrade.
should these functions be prefixed with v0_ to indicate intentions? Whilst renaming wont affect encoding based on call index, it may break integrations using the name obtained via metadata.
Renaming: makes sure integrations outside of contracts won't break.
Not renaming: makes the code cleaner and there is a high chance we will never need it. If we need it in the end we can change the naming and integrations outside of contracts will break.
- New contracts can use a
v1::fungibles::transfer
where they provide the extra parameter. This function will have a different dispatchable index and will call thev1_transfer()
8bc6576
to
f0fff97
Compare
f572fee
to
4a476b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Significant improvement in quality, well done! I think you can probably tell by my comments that I feel this is effectively done now, apart from a few finishing touches.
I think this is a good demonstration of how future implementations should be done and the expected levels of quality, cleanliness and simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconding Frank's comment, really good job on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats! 🚀 I know you put a lot of work into this, so thank you for the time and dedication. I hope you are pleased with the result.
I 100% agree, it is amazing how much it has improved the last couple of weeks.
I'm very pleased with how it turned and how much I learned. Thanks for all the reviews and help, it got to this point by us together. Looking forward to implementing the rest of improvements and features and get the first pop api use case complete, ready for hacking! |
Co-authored-by: Frank Bell <[email protected]>
Co-authored-by: Frank Bell <[email protected]>
Adds the api pallet implementing the psp22 interface. It resulted into an overall refactor of the pop api, mainly simplifying the implementation. Some small other relevant changes:
assets
tofungibles
feature in pop-api.contracts
inpop-api/integration-tests
which is contains the contracts for the integration tests.transfer_from
tests since it was a copy from `transfer. This is still to be implemented.Remaining todos:
transfer_from
anddecrease_allowance
#104approve