-
Notifications
You must be signed in to change notification settings - Fork 10
feat: fungibles precompiles #573
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
base: stable2506
Are you sure you want to change the base?
Conversation
ff4a829
to
8af8426
Compare
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## stable2506 #573 +/- ##
==============================================
+ Coverage 66.94% 69.10% +2.15%
==============================================
Files 125 134 +9
Lines 19107 21725 +2618
Branches 19107 21725 +2618
==============================================
+ Hits 12792 15012 +2220
- Misses 6073 6380 +307
- Partials 242 333 +91
🚀 New features to boost your workflow:
|
6f1248f
to
dff09e1
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.
Looks great overall
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.
Great work!
The structure of the solution is slightly different from what we previously had, more specifically, in stead of one precompile for "the entire runtime" we have a precompile per use case / interface. Example for the former:
#[inline]
pub fn transfer(id: TokenId, to: Address, value: U256) -> bool {
let result = build_call_solidity::<DefaultEnvironment>()
.call(POP_PRECOMPILE_ADDRESS) // Call Pop's precompile address.
.exec_input(
ExecutionInput::new(Selector::new([DISPATCH, V0, FUNGIBLES, TRANSFER])) // Use selector for `transfer`.
.push_arg(token)
.push_arg(to)
.push_arg(value),
)
.returns::<bool>()
.invoke();
}
Curious to hear what your reasoning has been
The erc20 standard has nothing about token creation or setting/clearing metadata as these are inherent to the contract implementing the standard. The fungibles interface is the wrapper over pallet-assets, the erc20 interface is a wrapper over an asset. Calling the erc20 precompile with the asset id encoded in the address makes an asset seem like an erc20. The fungibles precompile handles those functions which are not included in the erc20 standard. There is some overlap (e.g. transfer), but one can have different apis to the same underlying logic based on requirements/standards, rather than sticking them all in the same precompile. A PSP precompile could be added for such support as an example, which would just be a thin translation layer over existing pallet/runtime logic. The only constraint is the possible number of precompiles in the available address space. |
@evilrobot-01 Related to the address space for precompiles, I wonder why don't we have a generic precompile that accepts the pallet index, version, dispatchable name, and encoded parameters in |
The primary reason that comes to mind is that we don't need to define dispatchables, which cater to async execution for extrinsics with events as the mechanism for returning data, but rather allow any function call which allows for a better API for the contract. Why do we then need to retain the pallet dispatchable approach and encode information with pallet and call index when it was clearly limiting and hurting devex. No more additional queries for IDs, just return them. The precompile framework does all of this encoding automatically via the sol macro and an interface, eliminating a ton of code. My view is to complete the fungibles and only then assess optimisations. A variation on addressing can be layered on later if required, but feels an unnecessary distraction right now, which is embracing the new approach rather than clinging onto the old. |
Of course one could have a single precompile, but that would be like funneling everything through a single pipe, with extra overhead in doing all the encoding and decoding manually based on byte matching when the precompile framework does that for you already. My suggestion is to focus on completing the use cases and circle back to this as required. If there is a net reduction in code and complexity, then adding more complexity for the sake of it seems unwarranted. |
To support my concern why our current approach is a bit hard to sync with data structures between Solidity interfaces and Rust code. For example, in Solidity, optional parameter is not supported hence it requires additional methods to support But I agree there's a trade-off here. |
Solidity supports function overloading (https://docs.soliditylang.org/en/latest/contracts.html#function-overloading) so it should just be a case of an extra overload in the solidity interface to support it. The selector differs, so easy to define in the client side API implementation for Rust contracts. If the enum doesn't contain fields, it can be a u8 in the interface and then easily decoded/decoded into an enum. An enum with fields is just a series of tuples, so some manual encoding shouldn't be too hard. |
This is now as below, with additional safety checks on the contract side as they should be cheaper and more efficient than failing in the precompile. They can easily be stripped out once Solidity support is stablised and optimised, but as there is currently no support for custom errors in precompiles at least there is some better indication/protection from the contract library.
|
4fe1234
to
925537e
Compare
Rebased onto #582 |
1572333
to
9d6e22f
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.
Seems in a solid state now to progress to next round of reviews. The main hurdle remains around ink's lack of Solidity error handling support.
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.
The implementation looks super good! I like the use of contract_ref
in the library, makes it super clean. The more granular versioning is also a very nice improvement!
Didn't find anything concerning.
Can we also add a README to the new pop-api-vnext crate and fungibles example.
Would also be curious to see the contract size.
type Precompiles = ( | ||
// 1: `Fungibles` precompile v0 using `TrustBackedAssetsInstance` instances | ||
Fungibles<1, TrustBackedAssetsInstance>, | ||
// 2: `Erc20` precompile v0 using `TrustBackedAssetsInstance` instances | ||
Erc20<2, TrustBackedAssetsInstance>, | ||
); |
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.
So if a new version of e.g. the fungibles comes around, we will have to create another entry here, correct?
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.
Effectively yeah, as it will need to be accessed at another address to avoid breaking any existing contracts. If its just the addition of a new function to an existing precompile, should be able to add to an existing version.
One is free to use any number for the fixed address in the u16 space, so one could leave gaps based on potential versioning - e.g. 1-10 fungibles, 11-20 non-fungibles, 21-30 messaging etc. Its obviously far simpler to just use the next number, but that decision is out if the scope of this PR and can easily be adjusted to some convention prior to a release.
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.
Curious about the use of Erc20 configured for devnet runtime here. Fungibles
and Erc20
are simply just different in the interfaces provided so don't know if it necessary to also add Erc20 precompile to devnet runtime.
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.
They ultimately serve different purposes - the erc20 is just a subset of what is available in the fungibles, only as the fungibles api has been constructed that way previously. A minimal set of functions that allow a contract to interact with an asset via Erc20 standard, whereas the Fungibles api provides full functionality for creating and managing an asset beyond the standard. There may be cases where one only wants to expose a smaller API to a contract. It may also have lower contract sizes due to smaller trait definition, but that is just a guess.
Ultimately the current implementation simply tries to demonstrate multiple APIs over a single use case for maximum flexibility. As runtime owners/maintainers, feel free to configure only those precompiles you want, but a suggestion would be a deployment to ask for user feedback before prematurely deciding.
Technically, as the Erc20 is just a subset, one could have the erc20 trait definition pointing to the fungibles api precompile address. Having said that, as the sol! macro does not allow inheritance, there is no guaranteed way to ensure that the fungibles precompile has the required erc20 interface, apart from maybe tests. All that would be required is to then comment out the erc20 precompile in the runtime and change the index in the pop-api code to that of the fungibles and everything should 'just work'
In summary, the implementation was to show possibilities, which could then be easily configured/shaped as desired, rather than present a single API. Perhaps this is overkill, but the lift was minimal and it shows the potential of specific APIs with little effort.
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.
Please resolve this thread as appropriate.
c00106e
to
0021908
Compare
An adaptation of the existing fungibles API (pallet-api for the runtime/pop-api for ink contracts) to use precompiles. It adds new crates to minimize the disruption as much as possible until acceptance.
Pallet
The pallet does not contain any dispatchables, as its primary exposed interface are the precompiles which hook into
pallet-revive
. Ideally this would just be a crate, but minimal pallet setup is required due to benchmarking and pallet-assets instance configuration, which requires that it is added to the runtime. Ideally there would just be a single pallet with multiple submodules for each use-case, but multiple benchmarks implementations are not supported per pallet.NOTE: the implementation intentionally only uses Solidity encoding as a means to ensure that ink support exists. This is driven by the fact that precompiles require Solidity encoding.
The crate is structured as follows:
lib.rs
: general helpersfungibles
modulefungibles.rs
: thecore
implementation of the api, used by the precompiles and a minimal pallet definition. The intention here is to hold the core logic without worrying about Solidity types/encoding. Think of this layer simply as the dispatchables from the prior implementation which can be shared across different interfaces/precompiles.precompiles
moduleprecompiles/v0.rs
: includes the mainFungibles
API/precompile (with tests)erc20/v0.rs
: theErc20
precompile (with tests). An additional lightweight interface to a subset of the core logic.interfaces
: the solidity (.sol) interfacesbenchmarking.rs
: the ported benchmarks, adapted to call into a relevant precompile functiontests.rs
: the ported previous dispatchable tests, excluding events as these are emitted in the precompile rather than in the core pallet logic. None of the read tests were ported as they are included within the precompile/interface itself and therefore covered there instead.mock.rs
: uses new runtime syntax and default configs to keep as lean as possible.ExtBuilder
for building test externalities, which allows simplifying test state setup as much as possible.The crate is then added to the
devnet
runtime, with the two precompiles added to the pallet-revive config. A test shows the creation and minting of an asset via theFungibles
precompile, along with a transfer using theErc20
precompile. Note that it is not added as a pallet to the runtime as only the precompiles are required to inject into revive.Library
The ink library is largely the same, except that APIs are now versioned more granularly for greater flexbility. One shouldnt need to create a whole new version just to make a breaking change on a smaller component API. This could even go as far as smaller individual crates where they could be published independently based on need.
The crate is structured as follows:
fungibles
modulefungibles/v0.rs
:Fungibles
trait definition, along with additional function implementations which provide client-side validation. E.g. its more efficient to check for the zero address in the contract than to call into the runtime to do so. A developer could choose to just usecontract_ref!
directly if they dont want this. Errors and events are defined within submodules.erc20/v0.rs
:Erc20
trait definition, along with additional function implementations, errors and events submodules as above.sol.rs
: Solidity specific exportsintegration-tests
crate:fungibles.rs
: integration tests for theFungibles
precompile. TheErc20
precompile is an additional interface to the same functionality so probably doesnt warrant its own integration tests. The same Fungibles API is defined on the Contract helper struct. Ideally one would be able to use a contract_ref! directly in tests to avoid some of this boilerplate, but it tries to keep the encoding/decoding isolated to this type. The main benefit of this approach is clarity, where tests read better and are easier to follow as a result.contracts/fungibles
: port of existing fungibles test contract. Implements the sameFungibles
traits as defined for theFungibles
precompile.examples/fungibles
: existing fungibles example contract ported with a few improvements. Implements the sameErc20
andErc20Metadata
traits as defined for theErc20
precompile.Versioning
Versioning is incredibly simple. Each logical precompile should be independently versionable for maximum flexibility. The Solidity interfaces are therefore currently defined within
fungibles/precompiles/interfaces/v0', with the corresponding Rust implementation of pallet-revive's
Precompiletrait in a corresponding versioned module - e.g.
fungibles/precompiles/v0`, ensuring that a version of a precompile is fully self-contained. Namespacing is intentionally explicit, at least in terms of the pallet/runtime.Precompiles are added to the runtime by adding them with a specified index to the
Precompiles
config item ofpallet-revive
. These can be numbered sequentially, as is the case, or start at any number much like pallets are indexed within a runtime. Variants of a precompile could also be added based on an underlying instance as required.The pop-api client library is also versioned appropriately, with exports of the current version to make consumption from contracts simple. A contract should be able to use a prior version if desired, but there is perhaps no need for this once the crate is published.
TODOs:
examples/fungibles
: drink tests (blocked until drink support completed for ink! v6)[sc-3717]