feat(gnovm): add Coin constructor and more functionality

[leohhhn]

Description

This PR ports more functionality from coin.go, into Gno (std & stdshim). It will also update the concept & reference docs for Coins.

Superseding #1696

Contributors' checklist...

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests
• Added new benchmarks to generated graphs, if any. More info here.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 49.74%. Comparing base (ff61f86) to head (2b6fb4a).

Files	Patch %	Lines
gnovm/cmd/gno/transpile.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2104      +/-   ##
==========================================
+ Coverage   49.10%   49.74%   +0.64%     
==========================================
  Files         576      576              
  Lines       77821    77822       +1     
==========================================
+ Hits        38211    38712     +501     
+ Misses      36515    35981     -534     
- Partials     3095     3129      +34     

Flag	Coverage Δ
gno.land	61.62% <ø> (ø)
gnovm	44.28% <0.00%> (+2.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[leohhhn]

I have some thoughts about this:

Coins and their denoms are validated in the TM2 layer. The question is - should we also implement this validation in Gno, for example:

func NewCoin(denom string, amount int64) Coin {
    if err := validate(denom, amount); err != nil {
       panic(err)
    }

    return Coin{
       Denom:  denom,
       Amount: amount,
    }
}

or, is it maybe a better option to not do "double" validation since it spends gas? basically leave the validation and panicking to TM2 instead of the VM?

So, a constructor in Gno would be:

func NewCoin(denom string, amount int64) Coin {
    return Coin{
       Denom:  denom,
       Amount: amount,
    }
}

Need some input.

[leohhhn]

While we're on this topic, I came across this issue I made some time ago: #1676

Thinking about this a bit, we should really make a good, smooth constructor for the Coins struct.

One approach that I have is the following (in gnovm/stdlibs/std):

func NewCoins(val ...interface{}) Coins {
	if len(val)%2 != 0 {
		panic("invalid number of arguments")
	}

	cz := Coins{}
	for i := 0; i < len(val)-1; i++ {
		denom := val[i].(string) // todo validate?
		amt := val[i+1].(int64) // todo validate?
		cz = append(cz, NewCoin(denom, amt))
	}

	return cz
}

I'm aware that this is not the safest approach, but it's probably the most comfortable to use. With this, you can just do

coins := std.NewCoins("ugnot", 100, "othercoin", 200)

and its very painless, instead of coins:= std.Coins{std.Coin{ "ugnot", coinToSend }}).

WDYT?

[ltzmaxwell]

I have some thoughts about this:

Coins and their denoms are validated in the TM2 layer. The question is - should we also implement this validation in Gno, for example:

func NewCoin(denom string, amount int64) Coin {
    if err := validate(denom, amount); err != nil {
       panic(err)
    }

    return Coin{
       Denom:  denom,
       Amount: amount,
    }
}

or, is it maybe a better option to not do "double" validation since it spends gas? basically leave the validation and panicking to TM2 instead of the VM?

So, a constructor in Gno would be:

func NewCoin(denom string, amount int64) Coin {
    return Coin{
       Denom:  denom,
       Amount: amount,
    }
}

Need some input.

If validation isn't conducted within constructor of coin.gno, it'll be deferred to tm2 coin.go, leading to additional gas consumption due to extra operations. However, the efficiency depends on how frequently invalid denominations or amounts are set. This consideration is pivotal in determining the optimal approach. These are just initial thoughts, so I'm not entirely certain.

[leohhhn]

@ltzmaxwell thank you for your answer - it's possible that I wasn't fully clear - currently, there is no validation in the coins.gno file as it doesn't exist. So all validation is done in TM2 currently. The question is the following:

If we choose to add validation in coins.gno, will that not introduce double validation (on both the TM2 and Gno level) and thus use up more (double) the gas?

My question is the following: should we just not check at the Gno level, and leave the errors & panics to TM2 in this case, exactly to avoid using more gas than needed?

I will play around with this and measure the gas usage in both cases.

BTW, do you have any opinions on this comment?

[ltzmaxwell]

If we choose to add validation in coins.gno, will that not introduce double validation (on both the TM2 and Gno level) and thus use up more (double) the gas?

My question is the following: should we just not check at the Gno level, and leave the errors & panics to TM2 in this case, exactly to avoid using more gas than needed?

I will play around with this and measure the gas usage in both cases.

what I see in tm2/pkg/std.coin.go, the validation happens in Constructor and IsValid().

assume we are going to initiate a new coin with denominations of "Ugnot", which is obviously invalid,

• If validation is implemented at the gno level, the system will immediately identify the error and halt execution;
• Alternatively, if there is no gno level validation, the error will be caught later during the IsValid() check, leading to a halt in execution, albeit through a longer process.

On the other hand, I believe our priorities should be arranged as follows: security takes precedence, followed by readability and maintainability, and finally, gas consumption.

[ltzmaxwell]

While we're on this topic, I came across this issue I made some time ago: #1676

Thinking about this a bit, we should really make a good, smooth constructor for the Coins struct.

One approach that I have is the following (in gnovm/stdlibs/std):

func NewCoins(val ...interface{}) Coins {
	if len(val)%2 != 0 {
		panic("invalid number of arguments")
	}

	cz := Coins{}
	for i := 0; i < len(val)-1; i++ {
		denom := val[i].(string) // todo validate?
		amt := val[i+1].(int64) // todo validate?
		cz = append(cz, NewCoin(denom, amt))
	}

	return cz
}

I'm aware that this is not the safest approach, but it's probably the most comfortable to use. With this, you can just do

coins := std.NewCoins("ugnot", 100, "othercoin", 200)

and its very painless, instead of coins:= std.Coins{std.Coin{ "ugnot", coinToSend }}).

WDYT?

I like this.

[thehowl]

I did some changes in this PR, starting from wanting to verify that the fix in #2168 did indeed fix the issue we were having here

I saw some build issues and started investigating, but essentially stdshim was using math/overflow, which cannot be used right now because it is not in the stdlibWhitelist. (The error was not printed due to a swallowed error; this is fixed by #1695, but that PR is still stuck in review limbo)

I changed the stdshim version not to use math/overflow, so we can safely merge this for now; it matters little anyway (only makes the code "unsafe" for transpiled code, used literally nowhere), and it won't matter entirely after #1695 is merged.

[thehowl]

There are still some failing tests, but these seem in "userland" so I'll leave you to fix them

[thehowl]

There are some txtar tests failing because they're out of gas, due to the fact that adding std now uses more gas; just bump the count where needed.

[leohhhn]

@thehowl thanks for helping out. Fixed the tests, as well as @zivkovicmilos's comments.

[leohhhn]

Found a GnoVM bug: #2204

I implemented a duplicate check on the Coins ctor and added the docs: c82680a

[leohhhn]

Not sure why CI is failing, txtar integration tests are all passing locally.

[leohhhn]

Codecov broken? cc @ajnavarro