first part of addressing comments #578
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 23 files reviewed, all discussions resolved
…m/address-comments-v2
There was a problem hiding this comment.
Reviewable status: 0 of 23 files reviewed, 8 unresolved discussions
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
So, should we add the mainnet or remove the testnet? Also, if we remove the testnet - there is no tests in testcases - should I remove the whole test in this case?
There was a problem hiding this comment.
Reviewable status: 0 of 23 files reviewed, 8 unresolved discussions
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
Who know the answer?
There was a problem hiding this comment.
Reviewable status: 0 of 23 files reviewed, 8 unresolved discussions
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
So, what does or class is optional mean? Do you mean suffix in the function name?
There was a problem hiding this comment.
Reviewable status: 0 of 23 files reviewed, 8 unresolved discussions
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
So, what does
or class is optional mean? Do you mean suffix in the function name?
There was a problem hiding this comment.
Ignore this, I added the screenshot with full comment list in the end
Reviewable status: 0 of 23 files reviewed, 8 unresolved discussions
There was a problem hiding this comment.
Reviewable status: 0 of 23 files reviewed, 9 unresolved discussions (waiting on @vertex451)
README.md line 107 at r2 (raw file):
$ cored status --chain-id=coreum-testnet-1 --node=https://full-node.testnet-1.coreum.dev:26657
Why testnet here, I thought the example for the znet mostly, which uses the devnet prefix?
There was a problem hiding this comment.
Reviewable status: 0 of 23 files reviewed, 9 unresolved discussions (waiting on @dzmitryhil, @vertex451, and @ysv)
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
Proper name for this func seems to be GetBurntByClass
But I'm not sure if class arg is optional. If it is optional then current name is ok.
If it is required then should be renamed
There was a problem hiding this comment.
Reviewed 20 of 22 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dzmitryhil, @miladz68, @vertex451, and @wojtek-coreum)
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
so is it possible to simplify ?
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
saw in PR looks correct
Pls check other places also
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
So, should we add the mainnet or remove the testnet? Also, if we remove the testnet - there is no tests in testcases - should I remove the whole test in this case?
remove whole test I think
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
@miladz68 @dzmitryhil @wojtek-coreum WDYT ?
I think we agreed to hardcode everything because currently we have some values hardcoded (for denom) while other are still passed
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
pls do this change
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
Who know the answer?
maybe @miladz68 or @dzmitryhil ?
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
check this in other places
We should use smth like: assefttypes.CurrentTokenVersion directly here
README.md line 107 at r2 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Why testnet here, I thought the example for the znet mostly, which uses the devnet prefix?
I'm ok with testnet it is more stable
I think the issue was that chain-id is devnet but URL is testnet
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @miladz68, @vertex451, and @wojtek-coreum)
README.md line 107 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I'm ok with testnet it is more stable
I think the issue was that chain-id is devnet but URL is testnet
Ok.
There was a problem hiding this comment.
Reviewable status: 16 of 26 files reviewed, 8 unresolved discussions (waiting on @miladz68, @vertex451, @wojtek-coreum, and @ysv)
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
@miladz68 @dzmitryhil @wojtek-coreum WDYT ?
I think we agreed to hardcode everything because currently we have some values hardcoded (for denom) while other are still passed
Do you have a link to prev comment it's hard to understand based on the screenshot.
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
maybe @miladz68 or @dzmitryhil ?
Since by default use uses the SDK defaults, and the denom is stake.
There was a problem hiding this comment.
Reviewable status: 16 of 26 files reviewed, 8 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
saw in PR looks correct
Pls check other places also
Added better error assertion.
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
remove whole test I think
Removed
a discussion (no related file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Do you have a link to prev comment it's hard to understand based on the screenshot.
https://reviewable.io/reviews/CoreumFoundation/coreum/560#-N_3zYAxAoa69Y_3wpfg @dzmitryhil
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
pls do this change
Changed to:
// If the transfer is incoming or rate is nil or negative - return nil
if wibctransfertypes.IsPurposeIn(ctx) || rate.IsNil() || !rate.IsPositive() {
return nil
}
a discussion (no related file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Since by default use uses the SDK defaults, and the denom is stake.
So, no action items are needed, right?
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
Proper name for this func seems to be GetBurntByClass
But I'm not sure if class arg is optional. If it is optional then current name is ok.
If it is required then should be renamed
ClassID is used in CreateClassBurningKey, which returns an error in case of classID is not valid. So it is not optional, keeping the name GetBurntByClass.
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
check this in other places
We should use smth like:assefttypes.CurrentTokenVersiondirectly here
changed to assetfttypes.CurrentTokenVersion, thanks for the pointing out.
There was a problem hiding this comment.
Reviewable status: 16 of 26 files reviewed, 8 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
so is it possible to simplify ?
We cannot compare it this way because those classes are from different packages:
x/asset/nft/types/nft.pb.go and integration-tests/contracts/modules/nft.go
That's why we need reassigning
…m/address-comments-v2
There was a problem hiding this comment.
Reviewed 7 of 10 files at r3, 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @vertex451, and @wojtek-coreum)
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
Changed to:
// If the transfer is incoming or rate is nil or negative - return nil if wibctransfertypes.IsPurposeIn(ctx) || rate.IsNil() || !rate.IsPositive() { return nil }
If the transfer is incoming
but it is not just incoming it is IBC incoming
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @vertex451, and @wojtek-coreum)
a discussion (no related file):
Previously, vertex451 (Artem) wrote…
https://reviewable.io/reviews/CoreumFoundation/coreum/560#-N_3zYAxAoa69Y_3wpfg @dzmitryhil
Yes, we decided to keep it as is.
There was a problem hiding this comment.
Reviewed 7 of 22 files at r1, 4 of 10 files at r3, 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @miladz68 and @vertex451)
pkg/config/network_test.go line 167 at r5 (raw file):
} func TestGenesisCoreTotalSupply(t *testing.T) {
why is it removed?
x/asset/ft/keeper/before_send.go line 136 at r5 (raw file):
// amount * rate * min(non_issuer_inputs_sum, non_issuer_outputs_sum) / non_issuer_inputs_sum // If the transfer is incoming or rate is nil or negative - return nil
I think the IBC condition could be moved to separate if statement for clarity. The IBC-related comment could say this:
Neither burn rate nor send commission is charged on incoming IBC transfer.
And now Is ee we should do the same for rejected and timed-out transfers (!!!)
There was a problem hiding this comment.
Reviewed 4 of 22 files at r1, 2 of 3 files at r2, 3 of 10 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @miladz68 and @vertex451)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @miladz68 and @vertex451)
x/asset/ft/keeper/before_send.go line 136 at r5 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
I think the IBC condition could be moved to separate
ifstatement for clarity. The IBC-related comment could say this:Neither burn rate nor send commission is charged on incoming IBC transfer.And now Is ee we should do the same for rejected and timed-out transfers (!!!)
I will take care of it in different PR
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)
pkg/config/network_test.go line 167 at r5 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
why is it removed?
This line is present in my branch, could you specify?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
If the transfer is incomingbut it is not just incoming it is IBC incoming
Renamed to
// If the transfer is incoming from IBC or the rate is nil or negative - return nil
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, @vertex451, and @wojtek-coreum)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @vertex451)
pkg/config/network_test.go line 167 at r5 (raw file):
Previously, vertex451 (Artem) wrote…
This line is present in my branch, could you specify?
I see test func TestGenesisCoreTotalSupply is removed
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @ysv)
pkg/config/network_test.go line 167 at r5 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
I see test
func TestGenesisCoreTotalSupplyis removed
Ah, I see, yes, @ysv asked me to do it.
Here is the link to the discussion: https://reviewable.io/reviews/CoreumFoundation/coreum/578#-N_s4XH32mNkdvKfVf7j
@ysv could you please decide with @wojtek-coreum on this?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @ysv)
pkg/config/network_test.go line 167 at r5 (raw file):
Previously, vertex451 (Artem) wrote…
Ah, I see, yes, @ysv asked me to do it.
Here is the link to the discussion: https://reviewable.io/reviews/CoreumFoundation/coreum/578#-N_s4XH32mNkdvKfVf7j
@ysv could you please decide with @wojtek-coreum on this?
@ysv As I understand, because our genesis for mainnet and testnet are hardcoded, we don't need to verify it anymore, right?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)
pkg/config/network_test.go line 167 at r5 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
@ysv As I understand, because our genesis for mainnet and testnet are hardcoded, we don't need to verify it anymore, right?
yes, they are fixed forever so not really needed to run this anymore
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil and @miladz68)
This change is