-
Notifications
You must be signed in to change notification settings - Fork 302
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
auction: define action balances #4218
Conversation
// The sequence number should always be >= 1, because we can | ||
// only withdraw an auction that has ended (i.e. with sequence number `>=1`). | ||
// We use a saturating operation defensively so that we don't underflow. | ||
asset_id: AuctionNft::new(self.auction_id, self.seq.saturating_sub(1)).asset_id(), |
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.
@redshiftzero My rationale to do this is that we check the binding signature before we run any action validation checks, so even if this MUST be seq >= 1
, we can't actually rely on that yet.
pub fn new(id: AuctionId, seq: u64) -> AuctionNft { | ||
let metadata = asset::REGISTRY | ||
.parse_denom(&format!("auctionnft_{seq}_{id}")) | ||
.expect("auction nft denom is valid"); |
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.
I think I want to do a second pass on both auction and lp nfts to make their constructor fallible.
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.
Value balance changes LGTM! one small comment inline
Describe your changes
This PR:
auctionnft
s to the asset registryAuctionNft
with denom metadataDutchAuctionDescription::id
balance
for each actionIssue ticket number and link
This is a prelude to #4212
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: