gno icon indicating copy to clipboard operation
gno copied to clipboard

feat(stdlibs): remove support for linkedType in native bindings

Open thehowl opened this issue 1 year ago • 1 comments

Split from #1695 for ease of reviewing. Related to #814. Merge order:

  1. #1700 (this one!)
  2. #1702
  3. #1695

I scrapped the "linked identifier" feature of native bindings. The reasoning mostly stems from the changes subsequently implemented in #1695, as having "linked identifiers" means that:

  • the Gno linked types must have a different name from Go's if they are within the same package, so their names don't conflict after transpilation
  • in order for the "linked types" to match in the generated code, the AST has to be rewritten to make a type alias (ie. type Address = crypto.Bech32Address)
  • while still allowing the type to be "modifiable" by the std package, because we want to add our own methods -- this is not possible for imported types, obviously
  • and if we try removing methods, this creates errors because the methods on the original types don't match 1-1 those implemented in AST

Although, I think a decent case can be made besides easing our (my) life in transpiling:

  • Under the hood, the linked type mechanism works with Store.AddGno2GoMapping. This uses gonative (read: the bane of my existence -- #1361). If we remove all linked types, we can still pass data between Go and Gno using type literals, which don't incur in naming conflicts.
  • This also makes the workings of "native bindings" clearer in general, as they don't have any special linked types (which were currently hardcoded in the misc/genstd source)

Reviewing notes

  • Banker got severely refactored as it was mapping entire go interfaces into Go; it now uses simple elementary functions, with its old behaviour split between Go and Gno.
  • many other functions (std/native.gno) have also been changed so that their native function only uses primitive types (so everything that used an Address, now uses a string).
  • Due to the naming conflicts already mentioned, Go's Banker has been changed to BankerInterface so that it doesn't conflict.
  • AddGo2GnoMapping is unused in the codebase. This has been removed from Store to disencourage any further usage; removal of Go2Gno code is out of scope for this PR (see #1361)

thehowl avatar Feb 28 '24 14:02 thehowl

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 47.49%. Comparing base (01e91be) to head (3674d26).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1700      +/-   ##
==========================================
- Coverage   47.51%   47.49%   -0.03%     
==========================================
  Files         388      388              
  Lines       61373    61311      -62     
==========================================
- Hits        29159    29117      -42     
+ Misses      29772    29756      -16     
+ Partials     2442     2438       -4     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 28 '24 14:02 codecov[bot]