Define producer APIs #17

ethanfrey · 2023-04-27T20:54:30Z

Part of #2
Closes #3
Closes #4

Provide Rust APIs for how the contracts interact

hashedone

Mostly ok, but generated messages would contain obsolete _ signed before names.

.editorconfig

.gitignore

Cargo.toml

contracts/native-staking/src/contract.rs

hashedone · 2023-04-27T23:14:02Z

contracts/native-staking/src/types.rs

+use cosmwasm_schema::cw_serde;
+use cosmwasm_std::Addr;
+
+/*** state ***/


I would personally split this types.rs into state.rs (for state types) and msg.rs (for message-related-types - especially those are actual response messages).

I kind of agree.. but when I see lots of files of < 30 lines, it seems easier to join.

Can split them later on when they grow.

contracts/vault/src/contract.rs

JakeHartnell · 2023-04-28T21:07:55Z

contracts/native-staking/src/contract.rs

+        // instantiate or call stake on existing
+        todo!();
+    }
+


Kind of want a query to look up my proxy contract if it exists.

Same question. account seems wrong here, as this proxy handles only one address (that of the owner) over only one chain (that corresponding to denom).

The native-staking contract is a look up between owners and proxy contracts (and visa versa).
It will need to store that state, I will add queries for that

JakeHartnell · 2023-04-28T21:12:07Z

contracts/native-staking-proxy/src/contract.rs

+    /// TODO: can we do that with contract API?
+    /// Or better they use cosmjs native delegation queries with this proxy address
+    #[msg(query)]
+    fn unbonding(&self, _ctx: QueryCtx, _account: String) -> Result<ClaimsResponse, ContractError> {


Does this need the account argument? Only one per contract?

contracts/vault/src/contract.rs

JakeHartnell · 2023-04-28T21:33:17Z

contracts/native-staking/src/contract.rs

+    }
+
+    /// Returns the maximum percentage that can be slashed
+    /// TODO: any way to query this from the chain? or we just pass in InstantiateMsg???


Could probably do this with Stargate queries? A bit torn on what to do here... IMO querying it could help reduce edge cases. Perhaps we could make it an enum and have a struct that can handle the query logic?

Just spit-balling here...

pub enum MaxSlashPercentage { /// A fixed value, this cannot be changed for the life of the contract Hardcoded { max_slash: Decimal }, /// Queries the max percentage that can be slashed via Stargate StargateQuery { }, } pub struct MaxSlash { max_slash: MaxSlashPercentage } impl MaxSlash { fn get_max_slash_percentage (&self, ctx: ExecCtx) -> Result<Decimal, ContractError> { match self.max_slash { // If hardcoded we just return it MaxSlashPercentage::HardCoded { max_slash } => Ok(max_slash) // If not we do the stargate query MaxSlashPercentage::StargateQuery {} => { // do stargate query and return value } } } }

My hesitation with Stargate Queries is that they wouldn't work for chains using CosmWasm but not the SDK...

StargateQueries don't work for SDK chains either. They have been disabled for over a year since someone found non-determinism in them and halted Juno.

I will leave it as hardcoded for now, and make an issue for v1 to revisit this

JakeHartnell · 2023-04-28T21:38:35Z

contracts/vault/src/types.rs

+}
+
+#[cw_serde]
+pub struct BalanceResponse {


Nit on name, but this is returning more info than just a balance. Maybe AccountResponse and QueryMsg::Account?

packages/apis/src/local_staking_api.rs

packages/apis/src/vault_api.rs

JakeHartnell

Few minor comments, but this is the right direction from my perspective.

maurolacy

Looks good. Reviewed the native-staking-proxy contract. Will review the rest in another iteration.

maurolacy · 2023-04-29T08:20:39Z

contracts/native-staking-proxy/README.md

@@ -0,0 +1,17 @@
+# Native Staking Proxy


This 'proxy' staking contract would be the one referred as 'External Staking' in the Architecture doc, right?

I'll do a small PR on the Arch doc with some clarifications / details.

This is not yet reflected in that doc. We should probably update the flow charts to include it.

I just reference "Local Staking" in the doc, which is an interface.

"Native Staking" + "Native Staking Proxy" group would together implement the "Local Staking" functionality.

maurolacy · 2023-04-29T11:07:15Z

contracts/native-staking-proxy/src/contract.rs

+        }
+    }
+
+    /// The caller of the instantiation will be the native-staking contract.


According to the Architecture doc, the caller will be the vault contract. Can you clarify?

Ok, so the way this works is: vault -> native-staking -> native-staking-proxy.

The user story behind this is simple: as a user, I want to cross-stake my native asset as well as stake it to secure the native chain.

Why do we have the proxy? Because we want the user to be able to manage their native-stake just like they would normally (with the ability to vote, etc.). If all assets are sitting in one giant contract, this becomes difficult.

The caller of this proxy contract will be the native staking contract.

I see. It's called 'proxy' because it's an agent that acts on behalf of the user for some actions like unbonding, etc.

Thanks for the clarifications.

maurolacy · 2023-04-29T11:08:11Z

contracts/native-staking-proxy/src/contract.rs

+        &self,
+        ctx: InstantiateCtx,
+        denom: String,
+        owner: String,


Does this imply one proxy contract per user / address? This would be the original owner / provider of the funds, if I understand correctly.

Yes. One proxy contract per user.

Flow:

I deposit 1 $Juno into the vault

I decided to stake it to secure the Juno chain (not cross-staking yet)

A proxy account that I control is created for me where by I will be able to manage my stake

contracts/native-staking-proxy/src/contract.rs

maurolacy · 2023-04-29T11:10:56Z

contracts/native-staking-proxy/src/contract.rs

+        ctx: InstantiateCtx,
+        denom: String,
+        owner: String,
+        validator: String,


This is needed because of auto-staking, below. I think it makes sense for robustness, that any initially sent funds here are already being staked. As this is the entire purpose of this impl.

maurolacy · 2023-04-29T11:30:35Z

contracts/native-staking-proxy/src/contract.rs

+
+        // set owner as recipient of future withdrawls
+        let set_withdrawl = DistributionMsg::SetWithdrawAddress {
+            address: config.parent.into_string(),


Suggested change

address: config.parent.into_string(),

address: config.owner.into_string(),

?

If I understand this properly:

sender would (typically) be the vault contract, and becomes parent here.

owner would be (typically) set by the vault to the original user, the provider of the funds.

In principle it should be possible to instantiate one of these contracts directly, and that is not a problem. The vault would be just a convenient way to do it, AFAIK.

You are right.

Yes, typo. And good catch (why we need tests and real implementation). I just started sketching it out

maurolacy · 2023-04-29T11:45:24Z

contracts/native-staking/src/contract.rs

+        // instantiate or call stake on existing
+        todo!();
+    }
+


Same question. account seems wrong here, as this proxy handles only one address (that of the owner) over only one chain (that corresponding to denom).

contracts/native-staking-proxy/src/contract.rs

Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com>

maurolacy

LGTM. Only minor syntax / naming stuff.

maurolacy · 2023-05-02T06:30:12Z

contracts/native-staking-proxy/Cargo.toml

@@ -0,0 +1,42 @@
+[package]
+name = "mesh-native-staking-proxy"
+description = "Per-user proxy contract to manage your stake deposited into native-staking"


maurolacy · 2023-05-02T08:33:49Z

contracts/native-staking/README.md

+
+The user then interacts directly with the proxy up to the point of a complete unbonding, when they can send the now-liquid tokens
+to `native-staking`, which will immediately release those on the vault. This makes `native-staking` more a pass-through node and
+"air traffic controller", while logic to interact with staking and governance is placed inside the `native-staking-proxy`.


👍🏼 Good explanation.

contracts/vault/src/types.rs

maurolacy · 2023-05-02T09:26:13Z

contracts/vault/src/error.rs

+    #[error("The leinholder doesn't have any claims")]
+    UnknownLeinholder,


Here and elsewhere, do you mean 'lien'?

Suggested change

#[error("The leinholder doesn't have any claims")]

UnknownLeinholder,

#[error("The lien holder doesn't have any claims")]

UnknownLienholder,

Seems leinholder is an actual word from a quick search...

Hmm, it seems to me that the right or more common expression is 'lien holder'.

Mauro is correct. Lien is what I intended:

"a right to keep possession of property belonging to another person until a debt owed by that person is discharged"

maurolacy · 2023-05-02T09:27:00Z

contracts/vault/src/error.rs

+    #[error("The leinholder doesn't have enough claims for the action")]
+    InsufficientLein,


Suggested change

#[error("The leinholder doesn't have enough claims for the action")]

InsufficientLein,

#[error("The lien holder doesn't have enough claims for the action")]

InsufficientLien,

maurolacy · 2023-05-02T09:29:21Z

contracts/vault/src/types.rs

+pub struct LeinAddr {
+    pub leinholder: Addr,


Suggested change

pub struct LeinAddr {

pub leinholder: Addr,

pub struct LienAddr {

pub lien_holder: Addr,

contracts/vault/src/types.rs

maurolacy · 2023-05-02T09:31:05Z

contracts/vault/src/types.rs

+pub struct Lein {
+    pub leinholder: String,


Suggested change

pub struct Lein {

pub leinholder: String,

pub struct Lien {

pub lien_holder: String,

Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com>

ethanfrey added 3 commits April 27, 2023 21:56

Start defining packages

4cbc93e

Define Vault API

8cf226c

Define provider API types

feeeb87

ethanfrey changed the title ~~Define producer API~~ Define producer APIs Apr 27, 2023

ethanfrey added 2 commits April 27, 2023 23:15

Add stub for native staking contract

1648b8c

Add claim_rewards call

cc5ce87

hashedone reviewed Apr 27, 2023

View reviewed changes

ethanfrey added 6 commits April 28, 2023 09:12

Name RemoteStaking to CrossStaking

9f54d35

Write README on native-staking and update method calls.

c6e63b7

Start pulling out native-staking and proxy into two

aea75b9

Handle reply and cross-contract calls

6fddc5e

Many cleanup to make it proper sylvia usage of traits, etc

4a02614

Clean up native-staking and native-staking-proxy connections

48b6069

ethanfrey marked this pull request as ready for review April 28, 2023 10:09

JakeHartnell reviewed Apr 28, 2023

View reviewed changes

contracts/vault/src/contract.rs Outdated Show resolved Hide resolved

JakeHartnell reviewed Apr 28, 2023

View reviewed changes

packages/apis/src/local_staking_api.rs Outdated Show resolved Hide resolved

JakeHartnell reviewed Apr 28, 2023

View reviewed changes

packages/apis/src/vault_api.rs Outdated Show resolved Hide resolved

JakeHartnell approved these changes Apr 28, 2023

View reviewed changes

maurolacy reviewed Apr 29, 2023

View reviewed changes

contracts/native-staking-proxy/src/contract.rs Show resolved Hide resolved

Apply suggestions from code review

e4c52f8

Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com>

maurolacy approved these changes May 2, 2023

View reviewed changes

Apply suggestions from code review

10eebf2

Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com>

JakeHartnell merged commit ada7fd9 into main May 2, 2023

JakeHartnell deleted the define-producer-apis branch May 2, 2023 20:30

ethanfrey mentioned this pull request May 3, 2023

Finalize producer apis #19

Merged

2 tasks

	address: config.parent.into_string(),
	address: config.owner.into_string(),

		#[error("The leinholder doesn't have any claims")]
		UnknownLeinholder,

		#[error("The leinholder doesn't have enough claims for the action")]
		InsufficientLein,

Define producer APIs #17

Define producer APIs #17

Conversation

ethanfrey commented Apr 27, 2023 • edited Loading

hashedone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeHartnell left a comment

Choose a reason for hiding this comment

maurolacy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Apr 27, 2023 •

edited

Loading

ethanfrey Apr 28, 2023 •

edited

Loading

maurolacy Apr 29, 2023 •

edited

Loading